* Re: [BK PATCH] SCSI updates for 2.6.6 [not found] <20040518184527.GC4859@tpkurt.garloff.de> @ 2004-05-18 19:47 ` Guennadi Liakhovetski 2004-05-18 20:38 ` James Bottomley 2004-05-18 22:17 ` Matthew Wilcox 0 siblings, 2 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-18 19:47 UTC (permalink / raw) To: Kurt Garloff; +Cc: James Bottomley, Linux SCSI list, Linux kernel list On Tue, 18 May 2004, Kurt Garloff wrote: > On Tue, May 11, 2004 at 02:54:33PM -0500, James Bottomley wrote: > > On Tue, 2004-05-11 at 14:29, Guennadi Liakhovetski wrote: > > > I hoped the tmscsim 64-bit bugfix would somehow find its way into the > > > mainstream after 2.6. Does it still have a chance? > > > > The DC390 is a maintained driver: > > > > DC390/AM53C974 SCSI driver > > P: Kurt Garloff > > M: garloff@suse.de > > W: http://www.garloff.de/kurt/linux/dc390/ > > S: Maintained > > > > You need to get the maintainer to approve acceptance. > > Granted ;-) > > Actually, I had discussed the patch before with Guennadi and agreed to > it. As I don't really have much time any more for it, I would suggest > that Guennadi takes over, if he wants to. Well, Kurt, thanks for the offer. I am prepared and willing to do the work on supporting the driver, but I am, perhaps, not skilled enough to be a maintainer of a SCSI LDD. My knowledge of the SCSI protocol and the SCSI Linux subsystem is pretty limited. On one hand, the driver is little used, so, even if I badly break something it is not likely to cause major problems. On the other hand, I would feel more comfortable if, at least at the beginning, somebody would review my patches. Besides, it would be a good opportunity for me to really learn a bit more about SCSI, SCSI Linux driver, BIO,... oh well... Thanks Guennadi --- Guennadi Liakhovetski ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BK PATCH] SCSI updates for 2.6.6 2004-05-18 19:47 ` [BK PATCH] SCSI updates for 2.6.6 Guennadi Liakhovetski @ 2004-05-18 20:38 ` James Bottomley 2004-05-19 19:50 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski 2004-05-21 1:06 ` [BK PATCH] SCSI updates for 2.6.6 Chiaki 2004-05-18 22:17 ` Matthew Wilcox 1 sibling, 2 replies; 35+ messages in thread From: James Bottomley @ 2004-05-18 20:38 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Kurt Garloff, Linux SCSI list, Linux kernel list On Tue, 2004-05-18 at 14:47, Guennadi Liakhovetski wrote: > Well, Kurt, thanks for the offer. I am prepared and willing to do the work > on supporting the driver, but I am, perhaps, not skilled enough to be a > maintainer of a SCSI LDD. My knowledge of the SCSI protocol and the SCSI > Linux subsystem is pretty limited. On one hand, the driver is little used, > so, even if I badly break something it is not likely to cause major > problems. On the other hand, I would feel more comfortable if, at least at > the beginning, somebody would review my patches. Besides, it would be a > good opportunity for me to really learn a bit more about SCSI, SCSI Linux > driver, BIO,... oh well... OK, roll up for me what you'd currently like applied to the driver; anything that's less than solid, I'd rather mature in the -mm tree for a while. James ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH] tmscsim: 64-bit cleanup 2004-05-18 20:38 ` James Bottomley @ 2004-05-19 19:50 ` Guennadi Liakhovetski 2004-05-19 21:34 ` [PATCH] tmscsim: no internal queue Guennadi Liakhovetski 2004-05-21 15:15 ` [PATCH] tmscsim: 64-bit cleanup James Bottomley 2004-05-21 1:06 ` [BK PATCH] SCSI updates for 2.6.6 Chiaki 1 sibling, 2 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-19 19:50 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: TEXT/PLAIN, Size: 968 bytes --] On 18 May 2004, James Bottomley wrote: > OK, roll up for me what you'd currently like applied to the driver; > anything that's less than solid, I'd rather mature in the -mm tree for a > while. Ok, here comes the first one. I chose this one because it fixes an actual bug in the driver. This bug was (partially) introduced by myself when porting to 2.6. Partly the reason was that I disliked using function-like macros as lvalues: sg_dma_address(x) = ... sg_dma_len(x) = ... [OT] wouldn't it be better to introduce some macros like set_sg_dma_{address|len}(x, y)? A part of the original patch has already been merged (s/UINT/ULONG/), so, the actual version is re-diffed against 2.6.6-bk6 and re-tested (on a plain Pentium). Also updates the driver-version, printed on startup. Please, apply. Christoph, next I would like to see your patches applied. Do you want to re-submit them yourself, or should I do it for you? Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 5532 bytes --] diff -u linux-2.6.3/drivers/scsi/tmscsim.c linux-2.6.3.hardlink/drivers/scsi/tmscsim.c --- linux-2.6.3/drivers/scsi/tmscsim.c Fri Mar 12 22:22:11 2004 +++ linux-2.6.3.hardlink/drivers/scsi/tmscsim.c Tue Apr 13 22:50:05 2004 @@ -168,6 +168,7 @@ * 2.1a 03/11/29 GL, KG Initial fixing for 2.6. Convert to * * use the current PCI-mapping API, update * * command-queuing. * + * 2.1b 04/04/13 GL Fix for 64-bit platforms * ***********************************************************************/ /* Uncomment SA_INTERRUPT, if the driver refuses to share its IRQ with other devices */ @@ -1022,7 +1023,7 @@ pci_map_page(pdev, virt_to_page(pcmd->sense_buffer), (unsigned long)pcmd->sense_buffer & ~PAGE_MASK, sizeof(pcmd->sense_buffer), DMA_FROM_DEVICE); - pSRB->Segmentx.length = sizeof(pcmd->sense_buffer); + sg_dma_len(&pSRB->Segmentx) = sizeof(pcmd->sense_buffer); pSRB->SGcount = 1; pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; DEBUG1(printk("%s(): Mapped sense buffer %p at %x\n", __FUNCTION__, pcmd->sense_buffer, cmdp->saved_dma_handle)); @@ -1043,7 +1044,7 @@ (unsigned long)pcmd->request_buffer & ~PAGE_MASK, pcmd->request_bufflen, scsi_to_pci_dma_dir(pcmd->sc_data_direction)); /* TODO: error handling */ - pSRB->Segmentx.length = pcmd->request_bufflen; + sg_dma_len(&pSRB->Segmentx) = pcmd->request_bufflen; pSRB->SGcount = 1; pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; DEBUG1(printk("%s(): Mapped request buffer %p at %x\n", __FUNCTION__, pcmd->request_buffer, cmdp->saved_dma_handle)); @@ -1131,7 +1132,7 @@ if (!pcmd) { dc390_Free_insert (pACB, pSRB); return; } /* should not happen */ pDCB = dc390_findDCB (pACB, pcmd->device->id, pcmd->device->lun); if (!pDCB) - { + { dc390_Free_insert (pACB, pSRB); printk (KERN_ERR "DC390: Command in queue to non-existing device!\n"); pcmd->result = MK_RES(DRIVER_ERROR,DID_ERROR,0,0); @@ -1180,7 +1181,7 @@ /* Assume BAD_TARGET; will be cleared later */ cmd->result = DID_BAD_TARGET << 16; - /* TODO: Change the policy: Alway accept TEST_UNIT_READY or INQUIRY + /* TODO: Change the policy: Always accept TEST_UNIT_READY or INQUIRY * commands and alloc a DCB for the device if not yet there. DCB will * be removed in dc390_SRBdone if SEL_TIMEOUT */ @@ -1767,7 +1768,7 @@ PDCB pDCB, pDCB2; pDCB = kmalloc (sizeof(DC390_DCB), GFP_ATOMIC); - DCBDEBUG(printk (KERN_INFO "DC390: alloc mem for DCB (ID %i, LUN %i): %p\n" \ + DCBDEBUG(printk (KERN_INFO "DC390: alloc mem for DCB (ID %i, LUN %i): %p\n", \ id, lun, pDCB)); *ppDCB = pDCB; @@ -2229,10 +2230,13 @@ if ( PCI_PRESENT ) while (PCI_FIND_DEVICE (PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD53C974)) { -#if LINUX_VERSION_CODE > KERNEL_VERSION(2,3,30) if (pci_enable_device (pdev)) continue; -#endif + + if (pci_set_dma_mask(pdev, 0xffffffff)) { + printk(KERN_ERR "DC390(%i): No suitable DMA available.\n", dc390_adapterCnt); + continue; + } PCI_GET_IO_AND_IRQ; DEBUG0(printk(KERN_INFO "DC390(%i): IO_PORT=%04x,IRQ=%x\n", dc390_adapterCnt, (UINT) io_port, irq)); diff -u linux-2.6.3/drivers/scsi/scsiiom.c linux-2.6.3.hardlink/drivers/scsi/scsiiom.c --- linux-2.6.3/drivers/scsi/scsiiom.c Fri Mar 12 22:22:10 2004 +++ linux-2.6.3.hardlink/drivers/scsi/scsiiom.c Tue Apr 13 22:46:19 2004 @@ -521,7 +521,7 @@ { DC390_write8 (ScsiCmd, CLEAR_FIFO_CMD); DC390_write8 (DMA_Cmd, READ_DIRECTION+DMA_IDLE_CMD); /* | DMA_INT */ - } + } } static void @@ -740,9 +740,9 @@ psgl = pSRB->pSegmentList; //dc390_pci_sync(pSRB); - while (pSRB->TotalXferredLen + (ULONG) psgl->length < pSRB->Saved_Ptr) + while (pSRB->TotalXferredLen + (ULONG) sg_dma_len(psgl) < pSRB->Saved_Ptr) { - pSRB->TotalXferredLen += (ULONG) psgl->length; + pSRB->TotalXferredLen += (ULONG) sg_dma_len(psgl); pSRB->SGIndex++; if( pSRB->SGIndex < pSRB->SGcount ) { @@ -762,7 +762,7 @@ } else if(pcmd->request_buffer) { //dc390_pci_sync(pSRB); - pSRB->Segmentx.length = pcmd->request_bufflen - pSRB->Saved_Ptr; + sg_dma_len(&pSRB->Segmentx) = pcmd->request_bufflen - pSRB->Saved_Ptr; pSRB->SGcount = 1; pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; } else { @@ -885,6 +885,8 @@ if (pDCB) printk (KERN_ERR "DC390: pSRB == pTmpSRB! (TagQ Error?) (%02i-%i)\n", pDCB->TargetID, pDCB->TargetLUN); else printk (KERN_ERR "DC390: pSRB == pTmpSRB! (TagQ Error?) (DCB 0!)\n"); + + pSRB->pSRBDCB = pDCB; dc390_EnableMsgOut_Abort (pACB, pSRB); if (pDCB) pDCB->DCBFlag |= ABORT_DEV; return; @@ -1466,7 +1468,7 @@ ptr2 = pSRB->pSegmentList; for( i=pSRB->SGIndex; i < bval; i++) { - swlval += ptr2->length; + swlval += sg_dma_len(ptr2); ptr2++; } REMOVABLEDEBUG(printk(KERN_INFO "XferredLen=%08x,NotXferLen=%08x\n",\ diff -u linux-2.6.3/drivers/scsi/dc390.h linux-2.6.3.hardlink/drivers/scsi/dc390.h --- linux-2.6.3/drivers/scsi/dc390.h Fri Mar 12 22:22:10 2004 +++ linux-2.6.3.hardlink/drivers/scsi/dc390.h Tue Apr 13 22:50:26 2004 @@ -19,7 +19,7 @@ #endif #define DC390_BANNER "Tekram DC390/AM53C974" -#define DC390_VERSION "2.0f 2000-12-20" +#define DC390_VERSION "2.1b 2004-04-13" /* We don't have eh_abort_handler, eh_device_reset_handler, * eh_bus_reset_handler, eh_host_reset_handler yet! ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: no internal queue 2004-05-19 19:50 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski @ 2004-05-19 21:34 ` Guennadi Liakhovetski 2004-05-21 15:15 ` [PATCH] tmscsim: 64-bit cleanup James Bottomley 1 sibling, 0 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-19 21:34 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: TEXT/PLAIN, Size: 1312 bytes --] Here comes the 2nd one. I wanted to get Christoph's patches now in, but already the first his patch comes on the top of this one: remove internal command queuing in the driver. And, in fact, it fixes some bugs in it. So, they should go in together. Here's the original comment from Christoph's email of 1 Feb 2004: <start quote> Patch looks mostly good for me. But there's some fishyness in queuecommand, mostly from before you patch: - many failure cases return one with the new EH code although we wouldn't want to requeue the midlayer in that case. I removed the ifdef and added a failed goto to handle them. - we need to set cmd->result onlyh if we have an error instead of always an overriding it - else it will leak to the midlayer in the return 1 case. - the check for ids out of range are superflous (this was one of the errors above, I decided to remove it instead of fixing it). - you don't do DC390_UNLOCK_ACB when failing. While it's a noop I think it's bad to have locking macros in place and don't balance them. You should probably remove it completly in one of the next patches. - the pDCB->pWaitingSRB looks a bit strange. By unifying the the codepathes it becomes much more readable. <end quote> Please, apply. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 7336 bytes --] diff -ur linux-2.6.6-bk6/drivers/scsi/scsiiom.c linux-2.6.6-bk6-2/drivers/scsi/scsiiom.c --- linux-2.6.6-bk6/drivers/scsi/scsiiom.c Wed Feb 18 04:57:14 2004 +++ linux-2.6.6-bk6-2/drivers/scsi/scsiiom.c Thu Mar 4 21:21:39 2004 @@ -1152,7 +1152,6 @@ pSRB = psrb; } pDCB->pGoingSRB = 0; - dc390_Query_to_Waiting (pACB); dc390_Waiting_process (pACB); } else @@ -1634,7 +1633,6 @@ pcmd->scsi_done (pcmd); DC390_LOCK_ACB_NI; - dc390_Query_to_Waiting (pACB); dc390_Waiting_process (pACB); return; } @@ -1681,7 +1679,6 @@ pdcb->TagMask = 0; pdcb = pdcb->pNextDCB; } while( pdcb != pDCB ); - dc390_Query_to_Waiting (pACB); } diff -ur linux-2.6.6-bk6/drivers/scsi/tmscsim.c linux-2.6.6-bk6-2/drivers/scsi/tmscsim.c --- linux-2.6.6-bk6/drivers/scsi/tmscsim.c Wed Feb 18 05:00:00 2004 +++ linux-2.6.6-bk6-2/drivers/scsi/tmscsim.c Thu Mar 4 21:22:16 2004 @@ -169,6 +169,8 @@ * use the current PCI-mapping API, update * * command-queuing. * * 2.1b 04/04/13 GL Fix for 64-bit platforms * + * 2.1b1 04/01/31 GL (applied 05.04) Remove internal * + * command-queuing. * ***********************************************************************/ /* Uncomment SA_INTERRUPT, if the driver refuses to share its IRQ with other devices */ @@ -752,37 +754,6 @@ return q; } #endif - - -/* Append to Query List */ -static void dc390_Query_append( PSCSICMD cmd, PACB pACB ) -{ - dc390_cmd_scp_t *cmdq = (dc390_cmd_scp_t *)&cmd->SCp; - - DEBUG0(printk ("DC390: Append cmd %li to Query\n", cmd->pid)); - - list_add_tail(&cmdq->list, &pACB->cmdq); - pACB->QueryCnt++; - pACB->CmdOutOfSRB++; -} - - -/* Return next cmd from Query list */ -static PSCSICMD dc390_Query_get ( PACB pACB ) -{ - PSCSICMD pcmd; - dc390_cmd_scp_t *cmdq; - if (list_empty(&pACB->cmdq)) - return NULL; - - pcmd = (PSCSICMD) list_entry(pACB->cmdq.next, struct scsi_cmnd_list, scp.list); - DEBUG0(printk ("DC390: Get cmd %li from Query\n", pcmd->pid)); - cmdq = (dc390_cmd_scp_t *)&pcmd->SCp; - list_del(&cmdq->list); - pACB->QueryCnt--; - return pcmd; -} - /* Return next free SRB */ static __inline__ PSRB dc390_Free_get ( PACB pACB ) @@ -1114,37 +1085,6 @@ /* KG: deferred PCI mapping to dc390_StartSCSI */ } -/* Put cmnd from Query to Waiting list and send next Waiting cmnd */ -static void dc390_Query_to_Waiting (PACB pACB) -{ - Scsi_Cmnd *pcmd; - PSRB pSRB; - PDCB pDCB; - - if( pACB->ACBFlag & (RESET_DETECT+RESET_DONE+RESET_DEV) ) - return; - - while (pACB->QueryCnt) - { - pSRB = dc390_Free_get ( pACB ); - if (!pSRB) return; - pcmd = dc390_Query_get ( pACB ); - if (!pcmd) { dc390_Free_insert (pACB, pSRB); return; } /* should not happen */ - pDCB = dc390_findDCB (pACB, pcmd->device->id, pcmd->device->lun); - if (!pDCB) - { - dc390_Free_insert (pACB, pSRB); - printk (KERN_ERR "DC390: Command in queue to non-existing device!\n"); - pcmd->result = MK_RES(DRIVER_ERROR,DID_ERROR,0,0); - DC390_UNLOCK_ACB_NI; - pcmd->done (pcmd); - DC390_LOCK_ACB_NI; - } - dc390_BuildSRB (pcmd, pDCB, pSRB); - dc390_Waiting_append ( pDCB, pSRB ); - } -} - /*********************************************************************** * Function : static int DC390_queue_command (Scsi_Cmnd *cmd, * void (*done)(Scsi_Cmnd *)) @@ -1253,20 +1193,12 @@ pACB->Cmds++; cmd->scsi_done = done; cmd->result = 0; - - dc390_Query_to_Waiting (pACB); - - if( pACB->QueryCnt ) /* Unsent commands ? */ - { - DEBUG0(printk ("DC390: QueryCnt != 0\n")); - dc390_Query_append ( cmd, pACB ); - dc390_Waiting_process (pACB); - } - else if (pDCB->pWaitingSRB) + if (pDCB->pWaitingSRB) { pSRB = dc390_Free_get ( pACB ); DEBUG0(if (!pSRB) printk ("DC390: No free SRB but Waiting\n"); else printk ("DC390: Free SRB w/ Waiting\n")); - if (!pSRB) dc390_Query_append (cmd, pACB); + if (!pSRB) + return 1; else { dc390_BuildSRB (cmd, pDCB, pSRB); @@ -1280,8 +1212,8 @@ DEBUG0(if (!pSRB) printk ("DC390: No free SRB w/o Waiting\n"); else printk ("DC390: Free SRB w/o Waiting\n")); if (!pSRB) { - dc390_Query_append (cmd, pACB); dc390_Waiting_process (pACB); + return 1; } else { @@ -1499,21 +1431,6 @@ printk ("DC390: Abort command (pid %li, Device %02i-%02i)\n", cmd->pid, cmd->device->id, cmd->device->lun); - /* First scan Query list */ - if( pACB->QueryCnt ) - { - struct scsi_cmnd_list *t, *pcmd_l; - list_for_each_entry_safe(pcmd_l, t, &pACB->cmdq, scp.list) - if( (struct scsi_cmnd*)pcmd_l == cmd ) - { - /* Found: Dequeue */ - list_del(&pcmd_l->scp.list); - pACB->QueryCnt--; - status = SCSI_ABORT_SUCCESS; - goto ABO_X; - } - } - pDCB = dc390_findDCB (pACB, cmd->device->id, cmd->device->lun); if( !pDCB ) goto NOT_RUN; @@ -1971,8 +1888,6 @@ pACB->pActiveDCB = NULL; pACB->pFreeSRB = pACB->SRB_array; pACB->SRBCount = MAX_SRB_CNT; - pACB->QueryCnt = 0; - INIT_LIST_HEAD(&pACB->cmdq); pACB->AdapterIndex = index; pACB->status = 0; psh->this_id = dc390_eepromBuf[index][EE_ADAPT_SCSI_ID]; @@ -2772,7 +2687,6 @@ { int dev, spd, spd1; char *pos = buffer; - struct scsi_cmnd_list *cl; PACB pACB; PDCB pDCB; DC390_AFLAGS; @@ -2843,9 +2757,6 @@ SPRINTF (" %02i\n", pDCB->MaxCommand); pDCB = pDCB->pNextDCB; } - SPRINTF ("Commands in Queues: Query: %li:", pACB->QueryCnt); - list_for_each_entry(cl, &pACB->cmdq, scp.list) - SPRINTF (" %li", ((struct scsi_cmnd*)cl)->pid); if (timer_pending(&pACB->Waiting_Timer)) SPRINTF ("Waiting queue timer running\n"); else SPRINTF ("\n"); pDCB = pACB->pLinkDCB; diff -ur linux-2.6.6-bk6/drivers/scsi/tmscsim.h linux-2.6.6-bk6-2/drivers/scsi/tmscsim.h --- linux-2.6.6-bk6/drivers/scsi/tmscsim.h Wed Feb 18 04:59:25 2004 +++ linux-2.6.6-bk6-2/drivers/scsi/tmscsim.h Thu Mar 4 21:21:39 2004 @@ -214,16 +214,12 @@ PSRB pTmpSRB; /* 0x2c: */ -ULONG QueryCnt; -struct list_head cmdq; - -/* 0x38: */ UCHAR msgin123[4]; UCHAR DCBmap[MAX_SCSI_ID]; UCHAR Connected; UCHAR pad; -/* 0x3c: */ +/* 0x30: */ #if defined(USE_SPINLOCKS) && USE_SPINLOCKS > 1 && (defined(CONFIG_SMP) || DEBUG_SPINLOCKS > 0) spinlock_t lock; #endif @@ -234,20 +230,20 @@ UCHAR Ignore_IRQ; /* Not used */ PDEVDECL1; /* Pointer to PCI cfg. space */ -/* 0x4c/0x48: */ +/* 0x40/0x3c: */ ULONG Cmds; UINT SelLost; UINT SelConn; UINT CmdInQ; UINT CmdOutOfSRB; -/* 0x60/0x5c: */ +/* 0x54/0x50: */ struct timer_list Waiting_Timer; -/* 0x74/0x70: */ +/* 0x68/0x64: */ DC390_SRB TmpSRB; -/* 0xd8/0xd4: */ +/* 0xcc/0xc8: */ DC390_SRB SRB_array[MAX_SRB_CNT]; /* 50 SRBs */ -/* 0xfb0/0xfac: */ +/* 0xfa4/0xfa0: */ }; typedef struct _ACB DC390_ACB, *PACB; @@ -406,15 +402,8 @@ * SISC query queue */ typedef struct { - struct list_head list; dma_addr_t saved_dma_handle; } dc390_cmd_scp_t; - -struct scsi_cmnd_list -{ - char dummy[offsetof(struct scsi_cmnd, SCp)]; - dc390_cmd_scp_t scp; -}; /* ** Inquiry Data format [-- Attachment #3: Type: TEXT/PLAIN, Size: 4034 bytes --] diff -ur a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c --- a/drivers/scsi/tmscsim.c Wed May 19 22:42:45 2004 +++ b/drivers/scsi/tmscsim.c Wed May 19 23:03:25 2004 @@ -171,6 +171,7 @@ * 2.1b 04/04/13 GL Fix for 64-bit platforms * * 2.1b1 04/01/31 GL (applied 05.04) Remove internal * * command-queuing. * + * 2.1b2 04/02/01 CH (applied 05.04) Fix error-handling * ***********************************************************************/ /* Uncomment SA_INTERRUPT, if the driver refuses to share its IRQ with other devices */ @@ -1117,10 +1118,7 @@ cmd->cmnd[0],cmd->device->id,cmd->device->lun,cmd->pid, cmd->buffer)); DC390_LOCK_ACB; - - /* Assume BAD_TARGET; will be cleared later */ - cmd->result = DID_BAD_TARGET << 16; - + /* TODO: Change the policy: Always accept TEST_UNIT_READY or INQUIRY * commands and alloc a DCB for the device if not yet there. DCB will * be removed in dc390_SRBdone if SEL_TIMEOUT */ @@ -1131,17 +1129,6 @@ else if( (pACB->scan_devices) && (cmd->cmnd[0] == READ_6) ) pACB->scan_devices = 0; - if ( ( cmd->device->id >= pACB->pScsiHost->max_id ) || - (cmd->device->lun >= pACB->pScsiHost->max_lun) ) - { -/* printk ("DC390: Ignore target %d lun %d\n", - cmd->device->id, cmd->device->lun); */ - DC390_UNLOCK_ACB; - //return (1); - done (cmd); - return (0); - } - if( (pACB->scan_devices || cmd->cmnd[0] == TEST_UNIT_READY || cmd->cmnd[0] == INQUIRY) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun)) ) { @@ -1152,14 +1139,7 @@ { printk (KERN_ERR "DC390: kmalloc for DCB failed, target %02x lun %02x\n", cmd->device->id, cmd->device->lun); - DC390_UNLOCK_ACB; - printk ("DC390: No DCB in queue_command!\n"); -#ifdef USE_NEW_EH - return (1); -#else - done (cmd); - return (0); -#endif + goto fail; } } @@ -1167,10 +1147,7 @@ { printk(KERN_INFO "DC390: Ignore target %02x lun %02x\n", cmd->device->id, cmd->device->lun); - DC390_UNLOCK_ACB; - //return (1); - done (cmd); - return (0); + goto fail; } else { @@ -1179,52 +1156,37 @@ { /* should never happen */ printk (KERN_ERR "DC390: no DCB failed, target %02x lun %02x\n", cmd->device->id, cmd->device->lun); - DC390_UNLOCK_ACB; - printk ("DC390: No DCB in queuecommand (2)!\n"); -#ifdef USE_NEW_EH - return (1); -#else - done (cmd); - return (0); -#endif + goto fail; } } pACB->Cmds++; cmd->scsi_done = done; cmd->result = 0; - if (pDCB->pWaitingSRB) - { - pSRB = dc390_Free_get ( pACB ); - DEBUG0(if (!pSRB) printk ("DC390: No free SRB but Waiting\n"); else printk ("DC390: Free SRB w/ Waiting\n")); - if (!pSRB) - return 1; - else - { - dc390_BuildSRB (cmd, pDCB, pSRB); - dc390_Waiting_append (pDCB, pSRB); - } - dc390_Waiting_process (pACB); - } - else - { - pSRB = dc390_Free_get ( pACB ); - DEBUG0(if (!pSRB) printk ("DC390: No free SRB w/o Waiting\n"); else printk ("DC390: Free SRB w/o Waiting\n")); - if (!pSRB) - { - dc390_Waiting_process (pACB); - return 1; - } - else - { - dc390_BuildSRB (cmd, pDCB, pSRB); - dc390_SendSRB (pACB, pSRB); - } - } + + pSRB = dc390_Free_get(pACB); + if (!pSRB) + goto requeue; + + dc390_BuildSRB(cmd, pDCB, pSRB); + if (pDCB->pWaitingSRB) { + dc390_Waiting_append(pDCB, pSRB); + dc390_Waiting_process(pACB); + } else + dc390_SendSRB(pACB, pSRB); DC390_UNLOCK_ACB; DEBUG1(printk (KERN_DEBUG " ... command (pid %li) queued successfully.\n", cmd->pid)); return(0); + + requeue: + DC390_UNLOCK_ACB; + return 1; + fail: + DC390_UNLOCK_ACB; + cmd->result = DID_BAD_TARGET << 16; + done(cmd); + return 0; } /* We ignore mapping problems, as we expect everybody to respect ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-05-19 19:50 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski 2004-05-19 21:34 ` [PATCH] tmscsim: no internal queue Guennadi Liakhovetski @ 2004-05-21 15:15 ` James Bottomley 2004-05-21 22:53 ` Guennadi Liakhovetski ` (2 more replies) 1 sibling, 3 replies; 35+ messages in thread From: James Bottomley @ 2004-05-21 15:15 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Linux SCSI list, Christoph Hellwig On Wed, 2004-05-19 at 14:50, Guennadi Liakhovetski wrote: > Ok, here comes the first one. I chose this one because it fixes an actual > bug in the driver. This bug was (partially) introduced by myself when > porting to 2.6. Partly the reason was that I disliked using > function-like macros as lvalues: > > sg_dma_address(x) = ... > sg_dma_len(x) = ... > > [OT] wouldn't it be better to introduce some macros like > set_sg_dma_{address|len}(x, y)? Actually, no. struct scatterlist is for the OS platform to use to characterise DMA; it's actually a private structure entirely within the gift of the architecture to define. It usually contains bits of extraneous data that the driver never sees. The driver should convert struct scatterlist into its own version of the scatterlist that needs feeding to the hardware rather than try and use struct scatterlist. You'll find that lvalue use of sg_dma_len() fails on some platforms that actually compute it from page/offset values in the scatterlist. Could you (eventually) update tmscsim not to use it? Thanks, James ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-05-21 15:15 ` [PATCH] tmscsim: 64-bit cleanup James Bottomley @ 2004-05-21 22:53 ` Guennadi Liakhovetski 2004-05-22 8:09 ` Kurt Garloff 2004-06-04 22:17 ` Guennadi Liakhovetski 2 siblings, 0 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-21 22:53 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: TEXT/PLAIN, Size: 631 bytes --] On 21 May 2004, James Bottomley wrote: > You'll find that lvalue use of sg_dma_len() fails on some platforms that > actually compute it from page/offset values in the scatterlist. > > Could you (eventually) update tmscsim not to use it? Ok, thanks for the comments, I'll do that. Here comes the next (trivial) patch. I just want to get it out of the way to make the next (scary) patch smaller and simpler. This one doesn't modify the object code either. Actually, well, it does - it makes a few objects, that were previously declared extern static. And removes a couple more defines. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/plain, Size: 14202 bytes --] diff -u a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c --- a/drivers/scsi/tmscsim.c Thu May 20 23:06:54 2004 +++ b/drivers/scsi/tmscsim.c Fri May 21 23:14:48 2004 @@ -283,14 +283,8 @@ #define USE_SPINLOCKS 1 #define DC390_IFLAGS unsigned long iflags -#define DC390_DFLAGS unsigned long dflags -spinlock_t dc390_drvlock = SPIN_LOCK_UNLOCKED; #define DC390_LOCK_IO(dev) spin_lock_irqsave (((struct Scsi_Host *)dev)->host_lock, iflags) #define DC390_UNLOCK_IO(dev) spin_unlock_irqrestore (((struct Scsi_Host *)dev)->host_lock, iflags) -#define DC390_LOCK_DRV spin_lock_irqsave (&dc390_drvlock, dflags) -#define DC390_UNLOCK_DRV spin_unlock_irqrestore (&dc390_drvlock, dflags) -#define DC390_LOCK_DRV_NI spin_lock (&dc390_drvlock) -#define DC390_UNLOCK_DRV_NI spin_unlock (&dc390_drvlock) /* These macros are used for uniform access to 2.0.x and 2.1.x PCI config space*/ @@ -304,10 +298,7 @@ #define PCI_READ_CONFIG_BYTE(pd, rv, bv) pci_read_config_byte (pd, rv, bv) #define PCI_WRITE_CONFIG_WORD(pd, rv, bv) pci_write_config_word (pd, rv, bv) #define PCI_READ_CONFIG_WORD(pd, rv, bv) pci_read_config_word (pd, rv, bv) -#define PCI_BUS_DEV pdev->bus->number, pdev->devfn #define PCI_PRESENT (1) -#define PCI_SET_MASTER pci_set_master (pdev) -#define PCI_FIND_DEVICE(vend, id) (pdev = pci_find_device (vend, id, pdev)) #define PCI_GET_IO_AND_IRQ do{io_port = pci_resource_start (pdev, 0); irq = pdev->irq;} while(0) #include "tmscsim.h" @@ -316,38 +307,38 @@ # define __init #endif -UCHAR dc390_StartSCSI( PACB pACB, PDCB pDCB, PSRB pSRB ); -void dc390_DataOut_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); -void dc390_DataIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); +static UCHAR dc390_StartSCSI( PACB pACB, PDCB pDCB, PSRB pSRB ); +static void dc390_DataOut_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); +static void dc390_DataIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_Command_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_Status_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_MsgOut_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); -void dc390_MsgIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); +static void dc390_MsgIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_DataOutPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_DataInPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); -void dc390_CommandPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); +static void dc390_CommandPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_StatusPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); -void dc390_MsgOutPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); +static void dc390_MsgOutPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_MsgInPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_Nop_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_Nop_1( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_SetXferRate( PACB pACB, PDCB pDCB ); -void dc390_Disconnect( PACB pACB ); -void dc390_Reselect( PACB pACB ); -void dc390_SRBdone( PACB pACB, PDCB pDCB, PSRB pSRB ); -void dc390_DoingSRB_Done( PACB pACB, PSCSICMD cmd ); +static void dc390_Disconnect( PACB pACB ); +static void dc390_Reselect( PACB pACB ); +static void dc390_SRBdone( PACB pACB, PDCB pDCB, PSRB pSRB ); +static void dc390_DoingSRB_Done( PACB pACB, PSCSICMD cmd ); static void dc390_ScsiRstDetect( PACB pACB ); static void dc390_ResetSCSIBus( PACB pACB ); static void __inline__ dc390_RequestSense( PACB pACB, PDCB pDCB, PSRB pSRB ); static void __inline__ dc390_InvalidCmd( PACB pACB ); static void __inline__ dc390_EnableMsgOut_Abort (PACB, PSRB); static void dc390_remove_dev (PACB pACB, PDCB pDCB); -irqreturn_t do_DC390_Interrupt( int, void *, struct pt_regs *); +static irqreturn_t do_DC390_Interrupt( int, void *, struct pt_regs *); -int dc390_initAdapter( PSH psh, ULONG io_port, UCHAR Irq, UCHAR index ); -void dc390_initDCB( PACB pACB, PDCB *ppDCB, UCHAR id, UCHAR lun); -void dc390_updateDCB (PACB pACB, PDCB pDCB); +static int dc390_initAdapter( PSH psh, ULONG io_port, UCHAR Irq, UCHAR index ); +static void dc390_initDCB( PACB pACB, PDCB *ppDCB, UCHAR id, UCHAR lun); +static void dc390_updateDCB (PACB pACB, PDCB pDCB); static int DC390_release(struct Scsi_Host *host); static int dc390_shutdown (struct Scsi_Host *host); @@ -359,7 +350,7 @@ static UCHAR dc390_adapterCnt = 0; /* Startup values, to be overriden on the commandline */ -int tmscsim[] = {-2, -2, -2, -2, -2, -2}; +static int tmscsim[] = {-2, -2, -2, -2, -2, -2}; #if defined(MODULE) MODULE_PARM(tmscsim, "1-6i"); @@ -422,15 +413,15 @@ #endif /* Devices erroneously pretending to be able to do TagQ */ -UCHAR dc390_baddevname1[2][28] ={ +static UCHAR dc390_baddevname1[2][28] ={ "SEAGATE ST3390N 9546", "HP C3323-300 4269"}; #define BADDEVCNT 2 static char* dc390_adapname = "DC390"; -UCHAR dc390_eepromBuf[MAX_ADAPTER_NUM][EE_LEN]; -UCHAR dc390_clock_period1[] = {4, 5, 6, 7, 8, 10, 13, 20}; -UCHAR dc390_clock_speed[] = {100,80,67,57,50, 40, 31, 20}; +static UCHAR dc390_eepromBuf[MAX_ADAPTER_NUM][EE_LEN]; +static UCHAR dc390_clock_period1[] = {4, 5, 6, 7, 8, 10, 13, 20}; +static UCHAR dc390_clock_speed[] = {100,80,67,57,50, 40, 31, 20}; /*********************************************************************** * Functions for access to DC390 EEPROM @@ -495,7 +486,7 @@ } -int __initdata tmscsim_def[] = {7, 0 /* 10MHz */, +static int __initdata tmscsim_def[] = {7, 0 /* 10MHz */, PARITY_CHK_ | SEND_START_ | EN_DISCONNECT_ | SYNC_NEGO_ | TAG_QUEUEING_, MORE2_DRV | GREATER_1G | RST_SCSI_BUS | ACTIVE_NEGATION @@ -526,7 +517,7 @@ /* Override defaults on cmdline: * tmscsim: AdaptID, MaxSpeed (Index), DevMode (Bitmapped), AdaptMode (Bitmapped) */ -int __init dc390_setup (char *str) +static int __init dc390_setup (char *str) { int ints[8]; int i, im; @@ -818,7 +809,7 @@ dc390_Going_append (pDCB, pSRB); } -void DC390_waiting_timed_out (unsigned long ptr); +static void DC390_waiting_timed_out (unsigned long ptr); /* Sets the timer to wake us up */ static void dc390_waiting_timer (PACB pACB, unsigned long to) { @@ -871,7 +862,7 @@ } /* Wake up waiting queue */ -void DC390_waiting_timed_out (unsigned long ptr) +static void DC390_waiting_timed_out (unsigned long ptr) { PACB pACB = (PACB)ptr; DC390_IFLAGS; @@ -1046,7 +1037,7 @@ * ***********************************************************************/ -int DC390_queue_command (Scsi_Cmnd *cmd, void (* done)(Scsi_Cmnd *)) +static int DC390_queue_command (Scsi_Cmnd *cmd, void (* done)(Scsi_Cmnd *)) { PDCB pDCB; PSRB pSRB; @@ -1213,8 +1204,8 @@ * Note: * In contrary to other externally callable funcs (DC390_), we don't lock ***********************************************************************/ -int DC390_bios_param (struct scsi_device *sdev, struct block_device *bdev, - sector_t capacity, int geom[]) +static int DC390_bios_param (struct scsi_device *sdev, struct block_device *bdev, + sector_t capacity, int geom[]) { int heads, sectors, cylinders; PACB pACB = (PACB) sdev->host->hostdata; @@ -1250,15 +1241,14 @@ return (0); } #else -int DC390_bios_param (struct scsi_device *sdev, struct block_device *bdev, - sector_t capacity, int geom[]) +static int DC390_bios_param (struct scsi_device *sdev, struct block_device *bdev, + sector_t capacity, int geom[]) { return scsicam_bios_param (bdev, capacity, geom); } #endif - -void dc390_dumpinfo (PACB pACB, PDCB pDCB, PSRB pSRB) +static void dc390_dumpinfo (PACB pACB, PDCB pDCB, PSRB pSRB) { USHORT pstat; PDEVDECL1; if (!pDCB) pDCB = pACB->pActiveDCB; @@ -1312,7 +1302,7 @@ * Status: Buggy ! ***********************************************************************/ -int DC390_abort (Scsi_Cmnd *cmd) +static int DC390_abort (Scsi_Cmnd *cmd) { PDCB pDCB; PSRB pSRB, psrb; @@ -1518,7 +1508,7 @@ * Returns : 0 on success. ***********************************************************************/ -int DC390_reset (Scsi_Cmnd *cmd) +static int DC390_reset (Scsi_Cmnd *cmd) { UCHAR bval; PACB pACB = (PACB) cmd->device->host->hostdata; @@ -1567,7 +1557,7 @@ * Inputs : SCSI id and lun ***********************************************************************/ -void dc390_initDCB( PACB pACB, PDCB *ppDCB, UCHAR id, UCHAR lun ) +static void dc390_initDCB( PACB pACB, PDCB *ppDCB, UCHAR id, UCHAR lun ) { PEEprom prom; UCHAR index; @@ -1652,7 +1642,7 @@ * Purpose : Set the configuration dependent DCB parameters ***********************************************************************/ -void dc390_updateDCB (PACB pACB, PDCB pDCB) +static void dc390_updateDCB (PACB pACB, PDCB pDCB) { pDCB->SyncMode &= EN_TAG_QUEUEING | SYNC_NEGO_DONE /*| EN_ATN_STOP*/; if (pDCB->DevMode & TAG_QUEUEING_) { @@ -1690,7 +1680,7 @@ } -void dc390_linkSRB( PACB pACB ) +static void dc390_linkSRB( PACB pACB ) { UINT count, i; @@ -1715,7 +1705,7 @@ * io_port, Irq, index: Resources and adapter index ***********************************************************************/ -void __init dc390_initACB (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) +static void __init dc390_initACB (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) { PACB pACB; UCHAR i; @@ -1788,7 +1778,7 @@ * Outputs: 0 on success, -1 on error ***********************************************************************/ -int __init dc390_initAdapter (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) +static int __init dc390_initAdapter (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) { PACB pACB, pACB2; UCHAR dstate; @@ -1867,7 +1857,7 @@ * Inputs : host - pointer to this host adapter's structure * io_port - IO ports mapped to this adapter * Irq - IRQ assigned to this adpater - * PDEVDECL - PCI access handle + * struct pci_dev - PCI access handle * index - Adapter index * * Outputs: 0 on success, -1 on error @@ -1876,7 +1866,7 @@ * not in DC390_detect, called from outside ***********************************************************************/ -static int __init DC390_init (PSHT psht, ULONG io_port, UCHAR Irq, PDEVDECL, UCHAR index) +static int __init DC390_init (PSHT psht, ULONG io_port, UCHAR Irq, struct pci_dev *pdev, UCHAR index) { PSH psh; PACB pACB; @@ -1939,14 +1929,14 @@ int __init DC390_detect (Scsi_Host_Template *psht) { - PDEVDECL0; + struct pci_dev *pdev = NULL; UCHAR irq; ULONG io_port; dc390_pACB_start = NULL; if ( PCI_PRESENT ) - while (PCI_FIND_DEVICE (PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD53C974)) + while ((pdev = pci_find_device(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD53C974, pdev))) { if (pci_enable_device (pdev)) continue; @@ -1960,7 +1950,7 @@ if( !DC390_init(psht, io_port, irq, PDEV, dc390_adapterCnt)) { - PCI_SET_MASTER; + pci_set_master(pdev); dc390_set_pci_cfg (PDEV); dc390_adapterCnt++; } @@ -2002,8 +1992,8 @@ else SPRINTF(" No ") -int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start, - off_t offset, int length, int inout) +static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start, + off_t offset, int length, int inout) { int dev, spd, spd1; char *pos = buffer; @@ -2149,7 +2139,7 @@ return( 0 ); } -void dc390_freeDCBs (struct Scsi_Host *host) +static void dc390_freeDCBs (struct Scsi_Host *host) { PDCB pDCB, nDCB; PACB pACB = (PACB)(host->hostdata); @@ -2168,7 +2158,7 @@ } -int DC390_release (struct Scsi_Host *host) +static int DC390_release (struct Scsi_Host *host) { DC390_IFLAGS; PACB pACB = (PACB)(host->hostdata); diff -u a/drivers/scsi/dc390.h b/drivers/scsi/dc390.h --- a/drivers/scsi/dc390.h 12 Mar 2004 21:25:59 +++ b/drivers/scsi/dc390.h 21 May 2004 21:11:32 @@ -33,11 +33,11 @@ # define USE_NEW_EH #endif -extern int DC390_detect(Scsi_Host_Template *psht); -extern int DC390_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *)); -extern int DC390_abort(Scsi_Cmnd *cmd); -extern int DC390_reset(Scsi_Cmnd *cmd); -extern int DC390_bios_param(struct scsi_device *sdev, struct block_device *dev, +static int DC390_detect(Scsi_Host_Template *psht); +static int DC390_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *)); +static int DC390_abort(Scsi_Cmnd *cmd); +static int DC390_reset(Scsi_Cmnd *cmd); +static int DC390_bios_param(struct scsi_device *sdev, struct block_device *dev, sector_t capacity, int geom[]); static int DC390_release(struct Scsi_Host *); diff -u a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c --- a/drivers/scsi/scsiiom.c Thu May 20 22:49:29 2004 +++ b/drivers/scsi/scsiiom.c Fri May 21 23:42:35 2004 @@ -15,7 +15,7 @@ }; -UCHAR +static UCHAR dc390_StartSCSI( PACB pACB, PDCB pDCB, PSRB pSRB ) { UCHAR cmd; UCHAR disc_allowed, try_sync_nego; @@ -336,7 +336,7 @@ return IRQ_HANDLED; } -irqreturn_t do_DC390_Interrupt( int irq, void *dev_id, struct pt_regs *regs) +static irqreturn_t do_DC390_Interrupt( int irq, void *dev_id, struct pt_regs *regs) { irqreturn_t ret; DEBUG1(printk (KERN_INFO "DC390: Irq (%i) caught: ", irq)); @@ -346,7 +346,7 @@ return ret; } -void +static void dc390_DataOut_0( PACB pACB, PSRB pSRB, PUCHAR psstatus) { UCHAR sstatus; @@ -400,7 +400,7 @@ } } -void +static void dc390_DataIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus) { UCHAR sstatus, residual, bval; @@ -863,7 +863,7 @@ } -void +static void dc390_DataIO_Comm( PACB pACB, PSRB pSRB, UCHAR ioDir) { PSGL psgl; ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-05-21 15:15 ` [PATCH] tmscsim: 64-bit cleanup James Bottomley 2004-05-21 22:53 ` Guennadi Liakhovetski @ 2004-05-22 8:09 ` Kurt Garloff 2004-05-22 15:43 ` James Bottomley 2004-06-04 22:17 ` Guennadi Liakhovetski 2 siblings, 1 reply; 35+ messages in thread From: Kurt Garloff @ 2004-05-22 8:09 UTC (permalink / raw) To: James Bottomley; +Cc: Guennadi Liakhovetski, Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: text/plain, Size: 1159 bytes --] Hi James, Guennadi, On Fri, May 21, 2004 at 10:15:11AM -0500, James Bottomley wrote: > On Wed, 2004-05-19 at 14:50, Guennadi Liakhovetski wrote: > Actually, no. struct scatterlist is for the OS platform to use to > characterise DMA; it's actually a private structure entirely within the > gift of the architecture to define. It usually contains bits of > extraneous data that the driver never sees. The driver should convert > struct scatterlist into its own version of the scatterlist that needs > feeding to the hardware rather than try and use struct scatterlist. The AM53C974 hardware does not have hardware support for scatter-gather. (Yes, this is sad, you get an IRQ after every segment ...) > You'll find that lvalue use of sg_dma_len() fails on some platforms that > actually compute it from page/offset values in the scatterlist. > > Could you (eventually) update tmscsim not to use it? We could just define our own format, make a copy and then use that one. Regards, -- Kurt Garloff <garloff@suse.de> Cologne, DE SUSE LINUX AG / Novell, Nuernberg, DE Director SUSE Labs [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-05-22 8:09 ` Kurt Garloff @ 2004-05-22 15:43 ` James Bottomley 2004-05-22 18:10 ` Kurt Garloff ` (2 more replies) 0 siblings, 3 replies; 35+ messages in thread From: James Bottomley @ 2004-05-22 15:43 UTC (permalink / raw) To: Kurt Garloff; +Cc: Guennadi Liakhovetski, Linux SCSI list, Christoph Hellwig On Sat, 2004-05-22 at 03:09, Kurt Garloff wrote: > On Fri, May 21, 2004 at 10:15:11AM -0500, James Bottomley wrote: > > On Wed, 2004-05-19 at 14:50, Guennadi Liakhovetski wrote: > > Actually, no. struct scatterlist is for the OS platform to use to > > characterise DMA; it's actually a private structure entirely within the > > gift of the architecture to define. It usually contains bits of > > extraneous data that the driver never sees. The driver should convert > > struct scatterlist into its own version of the scatterlist that needs > > feeding to the hardware rather than try and use struct scatterlist. > > The AM53C974 hardware does not have hardware support for scatter-gather. > (Yes, this is sad, you get an IRQ after every segment ...) But then you should set sg_tablesize to one. There's little point the elevator wasting time to coalesce if you can't process the request in a single go. Doing that would also allow for a simpler driver design, because now you know you can complete individual requests since they only yield a single sg element. You no longer have to track multiple card completions to the request completion. > > You'll find that lvalue use of sg_dma_len() fails on some platforms that > > actually compute it from page/offset values in the scatterlist. > > > > Could you (eventually) update tmscsim not to use it? > > We could just define our own format, make a copy and then use that one. Sure, that would be fine. Whatever the registers take. James ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-05-22 15:43 ` James Bottomley @ 2004-05-22 18:10 ` Kurt Garloff 2004-05-22 19:34 ` [PATCH] tmscsim: new interfaces Guennadi Liakhovetski 2004-05-22 19:38 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski 2 siblings, 0 replies; 35+ messages in thread From: Kurt Garloff @ 2004-05-22 18:10 UTC (permalink / raw) To: James Bottomley; +Cc: Guennadi Liakhovetski, Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: text/plain, Size: 1243 bytes --] On Sat, May 22, 2004 at 10:43:12AM -0500, James Bottomley wrote: > On Sat, 2004-05-22 at 03:09, Kurt Garloff wrote: > > The AM53C974 hardware does not have hardware support for scatter-gather. > > (Yes, this is sad, you get an IRQ after every segment ...) > > But then you should set sg_tablesize to one. There's little point the > elevator wasting time to coalesce if you can't process the request in a > single go. Doing that would also allow for a simpler driver design, > because now you know you can complete individual requests since they > only yield a single sg element. You no longer have to track multiple > card completions to the request completion. It's still cheaper to send down one command for a large transfer, let the card interrupt the CPU to get a new DMA address every segment. (When we're lucky, we might even have segments with contiguous bus addresses -- this is certainly the case if there's a IOMMU in the system) The per-command overhead in SCSI is significant, so I would hate to restrict the driver to process 4k commands. Regards, -- Kurt Garloff <garloff@suse.de> Cologne, DE SUSE LINUX AG / Novell, Nuernberg, DE Director SUSE Labs [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-22 15:43 ` James Bottomley 2004-05-22 18:10 ` Kurt Garloff @ 2004-05-22 19:34 ` Guennadi Liakhovetski 2004-05-23 10:37 ` Christoph Hellwig 2004-05-22 19:38 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski 2 siblings, 1 reply; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-22 19:34 UTC (permalink / raw) To: James Bottomley; +Cc: Kurt Garloff, Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: TEXT/PLAIN, Size: 617 bytes --] Ok, here comes the risky (and, so far, the last) one. It updates the driver to use the "new" pci, scsi, and module interfaces. It is risky, because it changes quite a bit, and I, unfortunatly, can only test it in one configuration - compiled in (not as a module), and I don't have any devices, that support tagged command queues, and it is just an onboard AM53C974 chip without EEPROM. So, any amount of reviewing and testing would be appreciated. With a bit of work I could, perhaps, organise to boot from another media and so test a modular build, but not immediately. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 16212 bytes --] diff --exclude=CVS -ur linux-2.6.6/drivers/scsi/dc390.h ../linux-2.6.6-bk6-tmscsim/drivers/scsi/dc390.h --- linux-2.6.6/drivers/scsi/dc390.h Sat May 22 19:47:24 2004 +++ ../linux-2.6.6-bk6-tmscsim/drivers/scsi/dc390.h Sat May 22 00:05:08 2004 @@ -33,7 +33,6 @@ # define USE_NEW_EH #endif -static int DC390_detect(Scsi_Host_Template *psht); static int DC390_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *)); static int DC390_abort(Scsi_Cmnd *cmd); static int DC390_reset(Scsi_Cmnd *cmd); diff --exclude=CVS -ur linux-2.6.6/drivers/scsi/scsiiom.c ../linux-2.6.6-bk6-tmscsim/drivers/scsi/scsiiom.c --- linux-2.6.6/drivers/scsi/scsiiom.c Sat May 22 19:47:24 2004 +++ ../linux-2.6.6-bk6-tmscsim/drivers/scsi/scsiiom.c Sat May 22 12:11:26 2004 @@ -789,7 +789,7 @@ /* read and eval received messages */ -void +static void dc390_MsgIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus) { PDCB pDCB = pACB->pActiveDCB; @@ -948,7 +948,7 @@ dc390_DataIO_Comm (pACB, pSRB, READ_DIRECTION); } -void +static void dc390_CommandPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus) { PDCB pDCB; @@ -989,7 +989,7 @@ //DC390_write8 (DMA_Cmd, DMA_IDLE_CMD); } -void +static void dc390_MsgOutPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus) { UCHAR bval, i, cnt; @@ -1097,7 +1097,7 @@ } -void +static void dc390_Disconnect( PACB pACB ) { PDCB pDCB; @@ -1179,7 +1179,7 @@ } -void +static void dc390_Reselect( PACB pACB ) { PDCB pDCB; @@ -1349,10 +1349,10 @@ }; -void +static void dc390_SRBdone( PACB pACB, PDCB pDCB, PSRB pSRB ) { - UCHAR bval, status, i, DCB_removed; + UCHAR bval, status, i; PSCSICMD pcmd; PSCSI_INQDATA ptr; PSGL ptr2; @@ -1362,7 +1362,6 @@ /* KG: Moved pci_unmap here */ dc390_pci_unmap(pSRB); - DCB_removed = 0; status = pSRB->TargetStatus; ptr = (PSCSI_INQDATA) (pcmd->request_buffer); if( pcmd->use_sg ) @@ -1550,56 +1549,10 @@ pDCB->Inquiry7 = ptr->Flags; ckc_e: - if( pACB->scan_devices ) - { - if( pcmd->cmnd[0] == TEST_UNIT_READY || - pcmd->cmnd[0] == INQUIRY) - { -#ifdef DC390_DEBUG0 - printk (KERN_INFO "DC390: %s: result: %08x", - (pcmd->cmnd[0] == INQUIRY? "INQUIRY": "TEST_UNIT_READY"), - pcmd->result); - if (pcmd->result & (DRIVER_SENSE << 24)) printk (" (sense: %02x %02x %02x %02x)\n", - pcmd->sense_buffer[0], pcmd->sense_buffer[1], - pcmd->sense_buffer[2], pcmd->sense_buffer[3]); - else printk ("\n"); -#endif - if( (host_byte(pcmd->result) != DID_OK && !(status_byte(pcmd->result) & CHECK_CONDITION) && !(status_byte(pcmd->result) & BUSY)) || - ((driver_byte(pcmd->result) & DRIVER_SENSE) && (pcmd->sense_buffer[0] & 0x70) == 0x70 && - (pcmd->sense_buffer[2] & 0xf) == ILLEGAL_REQUEST) || host_byte(pcmd->result) & DID_ERROR ) - { - /* device not present: remove */ - //dc390_Going_remove (pDCB, pSRB); - dc390_remove_dev (pACB, pDCB); DCB_removed = 1; - - if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && - ((pcmd->device->lun == 0) || (pcmd->device->lun == pACB->pScsiHost->max_lun - 1)) ) - pACB->scan_devices = 0; - } - else - { - /* device present: add */ - if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && - (pcmd->device->lun == pACB->pScsiHost->max_lun - 1) ) - pACB->scan_devices = END_SCAN ; - /* pACB->DeviceCnt++; */ /* Dev is added on INQUIRY */ - } - } - } - - //if( pSRB->pcmd->cmnd[0] == INQUIRY && - // (host_byte(pcmd->result) == DID_OK || status_byte(pcmd->result) & CHECK_CONDITION) ) if( pcmd->cmnd[0] == INQUIRY && (pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) ) { - if ((ptr->DevType & SCSI_DEVTYPE) == TYPE_NODEV && !DCB_removed) - { - //printk ("DC390: Type = nodev! (%02i-%i)\n", pcmd->target, pcmd->lun); - /* device not present: remove */ - //dc390_Going_remove (pDCB, pSRB); - dc390_remove_dev (pACB, pDCB); DCB_removed = 1; - } - else + if ((ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV) { /* device found: add */ dc390_add_dev (pACB, pDCB, ptr); @@ -1608,11 +1561,11 @@ if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && (pcmd->device->lun == pACB->pScsiHost->max_lun - 1) ) pACB->scan_devices = 0; - }; + } pcmd->resid = pcmd->request_bufflen - pSRB->TotalXferredLen; - if (!DCB_removed) dc390_Going_remove (pDCB, pSRB); + dc390_Going_remove (pDCB, pSRB); /* Add to free list */ dc390_Free_insert (pACB, pSRB); @@ -1625,7 +1578,7 @@ /* Remove all SRBs from Going list and inform midlevel */ -void +static void dc390_DoingSRB_Done( PACB pACB, PSCSICMD cmd ) { PDCB pDCB, pdcb; @@ -1760,4 +1713,3 @@ if( pACB->pActiveDCB->pActiveSRB->SRBState & (SRB_START_+SRB_MSGOUT) ) DC390_write8 (ScsiCmd, CLEAR_FIFO_CMD); } - diff --exclude=CVS -ur linux-2.6.6/drivers/scsi/tmscsim.c ../linux-2.6.6-bk6-tmscsim/drivers/scsi/tmscsim.c --- linux-2.6.6/drivers/scsi/tmscsim.c Sat May 22 19:47:24 2004 +++ ../linux-2.6.6-bk6-tmscsim/drivers/scsi/tmscsim.c Sat May 22 00:22:29 2004 @@ -267,7 +267,6 @@ #include <linux/init.h> #include <linux/spinlock.h> -#if defined(MODULE) static struct pci_device_id tmscsim_pci_tbl[] = { { .vendor = PCI_VENDOR_ID_AMD, @@ -278,8 +277,7 @@ { } /* Terminating entry */ }; MODULE_DEVICE_TABLE(pci, tmscsim_pci_tbl); -#endif - + #define USE_SPINLOCKS 1 #define DC390_IFLAGS unsigned long iflags @@ -342,6 +340,8 @@ static int DC390_release(struct Scsi_Host *host); static int dc390_shutdown (struct Scsi_Host *host); +static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start, + off_t offset, int length, int inout); static PACB dc390_pACB_start= NULL; static PACB dc390_pACB_current = NULL; @@ -351,16 +351,14 @@ /* Startup values, to be overriden on the commandline */ static int tmscsim[] = {-2, -2, -2, -2, -2, -2}; +static int tmscsim_paramnum = ARRAY_SIZE(tmscsim); -#if defined(MODULE) -MODULE_PARM(tmscsim, "1-6i"); +module_param_array(tmscsim, int, tmscsim_paramnum, 0); MODULE_PARM_DESC(tmscsim, "Host SCSI ID, Speed (0=10MHz), Device Flags, Adapter Flags, Max Tags (log2(tags)-1), DelayReset (s)"); MODULE_AUTHOR("C.L. Huang / Kurt Garloff"); MODULE_DESCRIPTION("SCSI host adapter driver for Tekram DC390 and other AMD53C974A based PCI SCSI adapters"); MODULE_LICENSE("GPL"); - MODULE_SUPPORTED_DEVICE("sd,sr,sg,st"); -#endif static PVOID dc390_phase0[]={ dc390_DataOut_0, @@ -1051,41 +1049,29 @@ * commands and alloc a DCB for the device if not yet there. DCB will * be removed in dc390_SRBdone if SEL_TIMEOUT */ - if( (pACB->scan_devices == END_SCAN) && (cmd->cmnd[0] != INQUIRY) ) - pACB->scan_devices = 0; - - else if( (pACB->scan_devices) && (cmd->cmnd[0] == READ_6) ) + if (((pACB->scan_devices == END_SCAN) && (cmd->cmnd[0] != INQUIRY)) || + ((pACB->scan_devices) && (cmd->cmnd[0] == READ_6))) pACB->scan_devices = 0; - if( (pACB->scan_devices || cmd->cmnd[0] == TEST_UNIT_READY || cmd->cmnd[0] == INQUIRY) && - !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun)) ) - { - pACB->scan_devices = 1; - - dc390_initDCB( pACB, &pDCB, cmd->device->id, cmd->device->lun ); - if (!pDCB) - { - printk (KERN_ERR "DC390: kmalloc for DCB failed, target %02x lun %02x\n", - cmd->device->id, cmd->device->lun); - goto fail; - } - - } - else if( !(pACB->scan_devices) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun)) ) - { + if ((pACB->scan_devices || cmd->cmnd[0] == TEST_UNIT_READY || cmd->cmnd[0] == INQUIRY) && + !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) { + pACB->scan_devices = 1; + DCBDEBUG(printk("Scanning target %02x lun %02x\n", cmd->device->id, cmd->device->lun)); + } else if (!(pACB->scan_devices) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) { printk(KERN_INFO "DC390: Ignore target %02x lun %02x\n", cmd->device->id, cmd->device->lun); goto fail; } - else - { - pDCB = dc390_findDCB (pACB, cmd->device->id, cmd->device->lun); - if (!pDCB) - { /* should never happen */ - printk (KERN_ERR "DC390: no DCB failed, target %02x lun %02x\n", - cmd->device->id, cmd->device->lun); - goto fail; - } + + pDCB = dc390_findDCB (pACB, cmd->device->id, cmd->device->lun); + + /* Should it be: BUG_ON(!pDCB); ? */ + + if (!pDCB) + { /* should never happen */ + printk (KERN_ERR "DC390: no DCB found, target %02x lun %02x\n", + cmd->device->id, cmd->device->lun); + goto fail; } pACB->Cmds++; @@ -1856,7 +1842,7 @@ * * Inputs : host - pointer to this host adapter's structure * io_port - IO ports mapped to this adapter - * Irq - IRQ assigned to this adpater + * irq - IRQ assigned to this adpater * struct pci_dev - PCI access handle * index - Adapter index * @@ -1865,10 +1851,8 @@ * Note: written in capitals, because the locking is only done here, * not in DC390_detect, called from outside ***********************************************************************/ - -static int __init DC390_init (PSHT psht, ULONG io_port, UCHAR Irq, struct pci_dev *pdev, UCHAR index) +static int __init dc390_init (PSH psh, unsigned long io_port, u8 irq, struct pci_dev *pdev, UCHAR index) { - PSH psh; PACB pACB; if (dc390_CheckEEpromCheckSum (PDEV, index)) @@ -1890,25 +1874,16 @@ dc390_check_for_safe_settings (); dc390_EEprom_Override (index); } - - psh = scsi_register( psht, sizeof(DC390_ACB) ); - if( !psh ) return( -1 ); - - scsi_set_device(psh, &pdev->dev); pACB = (PACB) psh->hostdata; DEBUG0(printk(KERN_INFO "DC390: pSH = %8x, Index %02i\n", (UINT) psh, index)); - dc390_initACB( psh, io_port, Irq, index ); + dc390_initACB( psh, io_port, irq, index ); PDEVSET; - if( !dc390_initAdapter( psh, io_port, Irq, index ) ) + if( !dc390_initAdapter( psh, io_port, irq, index ) ) { - DEBUG0(printk("DC390: pACB = %8x, pDCBmap = %8x, pSRB_array = %8x\n",\ - (UINT) pACB, (UINT) pACB->DCBmap, (UINT) pACB->SRB_array)); - DEBUG0(printk("DC390: ACB size= %4x, DCB size= %4x, SRB size= %4x\n",\ - sizeof(DC390_ACB), sizeof(DC390_DCB), sizeof(DC390_SRB) )); return (0); } else @@ -1927,42 +1902,110 @@ PCI_WRITE_CONFIG_WORD (PDEV, PCI_STATUS, (PCI_STATUS_SIG_SYSTEM_ERROR | PCI_STATUS_DETECTED_PARITY)); } -int __init DC390_detect (Scsi_Host_Template *psht) +/** + * dc390_slave_alloc - Called by the scsi mid layer to tell us about a new + * scsi device that we need to deal with. + * + * @scsi_device: The new scsi device that we need to handle. + */ +static int dc390_slave_alloc(struct scsi_device *scsi_device) { - struct pci_dev *pdev = NULL; - UCHAR irq; - ULONG io_port; - - dc390_pACB_start = NULL; + PDCB pDCB; + PACB pACB = (PACB) scsi_device->host->hostdata; + dc390_initDCB(pACB, &pDCB, scsi_device->id, scsi_device->lun); + if (pDCB != NULL) + return 0; + return -ENOMEM; +} - if ( PCI_PRESENT ) - while ((pdev = pci_find_device(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD53C974, pdev))) - { - if (pci_enable_device (pdev)) - continue; +/** + * dc390_slave_destroy - Called by the scsi mid layer to tell us about a + * device that is going away. + * + * @scsi_device: The scsi device that we need to remove. + */ +static void dc390_slave_destroy(struct scsi_device *scsi_device) +{ + PACB pACB = (PACB) scsi_device->host->hostdata; + PDCB pDCB = dc390_findDCB (pACB, scsi_device->id, scsi_device->lun);; + if (pDCB != NULL) + dc390_remove_dev(pACB, pDCB); + else + printk(KERN_ERR"%s() called for non-existing device!\n", __FUNCTION__); +} + +static Scsi_Host_Template driver_template = { + .module = THIS_MODULE, + .proc_name = "tmscsim", + .proc_info = DC390_proc_info, + .name = DC390_BANNER " V" DC390_VERSION, + .slave_alloc = dc390_slave_alloc, + .slave_destroy = dc390_slave_destroy, + .queuecommand = DC390_queue_command, + .eh_abort_handler = DC390_abort, + .eh_bus_reset_handler = DC390_reset, + .bios_param = DC390_bios_param, + .can_queue = 42, + .this_id = 7, + .sg_tablesize = SG_ALL, + .cmd_per_lun = 16, + .use_clustering = DISABLE_CLUSTERING, +}; - if (pci_set_dma_mask(pdev, 0xffffffff)) { - printk(KERN_ERR "DC390(%i): No suitable DMA available.\n", dc390_adapterCnt); - continue; - } - PCI_GET_IO_AND_IRQ; - DEBUG0(printk(KERN_INFO "DC390(%i): IO_PORT=%04x,IRQ=%x\n", dc390_adapterCnt, (UINT) io_port, irq)); +static int __devinit dc390_init_one(struct pci_dev *dev, + const struct pci_device_id *id) +{ + struct Scsi_Host *scsi_host; + unsigned long io_port; + u8 irq; + PACB pACB; + + if (pci_enable_device(dev)) + return -ENODEV; + + io_port = pci_resource_start(dev, 0); + irq = dev->irq; + + /* allocate scsi host information (includes out adapter) */ + scsi_host = scsi_host_alloc(&driver_template, sizeof(struct _ACB)); + if (!scsi_host) + return -ENOMEM; + + pACB = (PACB)scsi_host->hostdata; + + if (dc390_init(scsi_host, io_port, irq, dev, dc390_adapterCnt)) { + scsi_host_put(scsi_host); + return -EBUSY; + } - if( !DC390_init(psht, io_port, irq, PDEV, dc390_adapterCnt)) - { - pci_set_master(pdev); - dc390_set_pci_cfg (PDEV); - dc390_adapterCnt++; - } + pci_set_master(dev); + dc390_set_pci_cfg(dev); + dc390_adapterCnt++; + + /* get the scsi mid level to scan for new devices on the bus */ + if (scsi_add_host(scsi_host, &dev->dev)) { + scsi_host_put(scsi_host); + return -ENODEV; } - else - printk (KERN_ERR "DC390: No PCI BIOS found!\n"); - - if (dc390_adapterCnt) - psht->proc_name = "tmscsim"; + pci_set_drvdata(dev, scsi_host); + scsi_scan_host(scsi_host); - printk(KERN_INFO "DC390: %i adapters found\n", dc390_adapterCnt); - return( dc390_adapterCnt ); + return 0; +} + +/** + * dc390_remove_one - Called to remove a single instance of the adapter. + * + * @dev: The PCI device to remove. + */ +static void __devexit dc390_remove_one(struct pci_dev *dev) +{ + struct Scsi_Host *scsi_host = pci_get_drvdata(dev); + + scsi_remove_host(scsi_host); + DC390_release(scsi_host); + scsi_host_put(scsi_host); + pci_set_drvdata(dev, NULL); } /******************************************************************** @@ -2177,24 +2220,25 @@ release_region(host->io_port,host->n_io_port); dc390_freeDCBs (host); DC390_UNLOCK_IO(host); - scsi_unregister(host); return( 1 ); } -static Scsi_Host_Template driver_template = { - .proc_name = "tmscsim", - .proc_info = DC390_proc_info, - .name = DC390_BANNER " V" DC390_VERSION, - .detect = DC390_detect, - .release = DC390_release, - .queuecommand = DC390_queue_command, - .eh_abort_handler = DC390_abort, - .eh_bus_reset_handler = DC390_reset, - .bios_param = DC390_bios_param, - .can_queue = 42, - .this_id = 7, - .sg_tablesize = SG_ALL, - .cmd_per_lun = 16, - .use_clustering = DISABLE_CLUSTERING, +static struct pci_driver dc390_driver = { + .name = "tmscsim", + .id_table = tmscsim_pci_tbl, + .probe = dc390_init_one, + .remove = __devexit_p(dc390_remove_one), }; -#include "scsi_module.c" + +static int __init dc390_module_init(void) +{ + return pci_module_init(&dc390_driver); +} + +static void __exit dc390_module_exit(void) +{ + pci_unregister_driver(&dc390_driver); +} + +module_init(dc390_module_init); +module_exit(dc390_module_exit); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-22 19:34 ` [PATCH] tmscsim: new interfaces Guennadi Liakhovetski @ 2004-05-23 10:37 ` Christoph Hellwig 2004-05-23 21:19 ` Guennadi Liakhovetski ` (2 more replies) 0 siblings, 3 replies; 35+ messages in thread From: Christoph Hellwig @ 2004-05-23 10:37 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: James Bottomley, Kurt Garloff, Linux SCSI list, Christoph Hellwig On Sat, May 22, 2004 at 09:34:47PM +0200, Guennadi Liakhovetski wrote: > Ok, here comes the risky (and, so far, the last) one. It updates the > driver to use the "new" pci, scsi, and module interfaces. It is risky, > because it changes quite a bit, and I, unfortunatly, can only test it in > one configuration - compiled in (not as a module), and I don't have any > devices, that support tagged command queues, and it is just an onboard > AM53C974 chip without EEPROM. Looks mostly goo to me, one thing I worry about is the complete removal of the scan_device flag, I don't fully understand what it's supposed to do, but we should be able to always set in ->slave_alloc and then clear it in ->slave_configure when we know probing is over for this device. The other thing is that most calls to dc390_findDCB could probably be replaced with stroing the pDCB in scsi_device->hostdata on slave_alloc, but that could aswell be done in an incremental patch. In dc390_init_one you don't call pci_disable_device on failure, best switch to goto for a single shared error path to avoid all these leaks. Maybe also merge dc390_init and dc390_init_one? Similarly I think DC390_release should be merged into dc390_remove_one. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-23 10:37 ` Christoph Hellwig @ 2004-05-23 21:19 ` Guennadi Liakhovetski 2004-05-26 11:45 ` Christoph Hellwig 2004-05-30 21:01 ` [PATCH] tmscsim: Store pDCB in device->hostdata Guennadi Liakhovetski 2004-05-31 21:13 ` [PATCH] tmscsim: init / exit cleanup Guennadi Liakhovetski 2 siblings, 1 reply; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-23 21:19 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list [-- Attachment #1: Type: TEXT/PLAIN, Size: 2677 bytes --] On Sun, 23 May 2004, Christoph Hellwig wrote: > On Sat, May 22, 2004 at 09:34:47PM +0200, Guennadi Liakhovetski wrote: > > Ok, here comes the risky (and, so far, the last) one. It updates the > > driver to use the "new" pci, scsi, and module interfaces. It is risky, > > because it changes quite a bit, and I, unfortunatly, can only test it in > > one configuration - compiled in (not as a module), and I don't have any > > devices, that support tagged command queues, and it is just an onboard > > AM53C974 chip without EEPROM. > > Looks mostly goo to me, one thing I worry about is the complete > removal of the scan_device flag, I don't fully understand what it's > supposed to do, but we should be able to always set in ->slave_alloc > and then clear it in ->slave_configure when we know probing is over > for this device. Right, thanks. I didn't understand at all what scan_devices was doing there, so, I tried to preserve its semantics in the driver, and "just" overlooked its setting in dc390_SRBdone:-( But, wouldn't it be safer to just restore the relevant parts there, rather than moving it in slave_alloc / configure? Because... emn, well, I wanted to write "because it is tested later on in dc390_SRBdone: if (pACB->scan_devices) pACB->DeviceCnt++; ", but then I checked, where else this DeviceCnt is used, and found only initialisation to 0, the increment above, no decrement, and a proc-output: SPRINTF("Nr of attached devices: %i, Nr of DCBs: %i\n", pACB->DeviceCnt, pACB->DCBCnt); Whereas, DCBCnt is incremented on slave_alloc, decremented on slave_destroy and used in a couple more places. So, is it safe to assume that DeviceCnt is redundant and remove it in a next patch?... Then it might be already safe to move scan_devices as you suggest? > The other thing is that most calls to dc390_findDCB could probably be > replaced with stroing the pDCB in scsi_device->hostdata on slave_alloc, > but that could aswell be done in an incremental patch. Ok, one more item for the todo list, when I start it... > In dc390_init_one you don't call pci_disable_device on failure, > best switch to goto for a single shared error path to avoid all > these leaks. Done. Actually, I copied this (and other) new function(s) from dc395x, so, they have this bug too... And, I asume, it has to be done in remove_one too, right? > Maybe also merge dc390_init and dc390_init_one? > > Similarly I think DC390_release should be merged into dc390_remove_one. Ok, 2 more items... So, attached is version 2 of the patch. Besides, there's a tiny trivial (and, hopefully, correct) patch to scsi_host.h, fixing the comment. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 1397 bytes --] Index: include/scsi/scsi_host.h =================================================================== RCS file: /usr/src/cvs/linux-2_6/include/scsi/scsi_host.h,v retrieving revision 1.1.1.3 diff -u -r1.1.1.3 scsi_host.h --- include/scsi/scsi_host.h 19 May 2004 16:08:49 -0000 1.1.1.3 +++ include/scsi/scsi_host.h 23 May 2004 20:18:31 -0000 @@ -151,7 +151,7 @@ * here then you will get a call to slave_configure(), then the * device will be used for however long it is kept around, then when * the device is removed from the system (or * possibly at reboot - * time), you will then get a call to slave_detach(). This is + * time), you will then get a call to slave_destroy(). This is * assuming you implement slave_configure and slave_destroy. * However, if you allocate memory and hang it off the device struct, * then you must implement the slave_destroy() routine at a minimum @@ -185,7 +185,7 @@ * specific setup basis... * 6. Return 0 on success, non-0 on error. The device will be marked * as offline on error so that no access will occur. If you return - * non-0, your slave_detach routine will never get called for this + * non-0, your slave_destroy routine will never get called for this * device, so don't leave any loose memory hanging around, clean * up after yourself before returning non-0 * [-- Attachment #3: Type: TEXT/PLAIN, Size: 15480 bytes --] diff -ur --exclude=CVS vanilla/linux-2.6.6/drivers/scsi/dc390.h linux-2.6.6-bk6-tmscsim/drivers/scsi/dc390.h --- vanilla/linux-2.6.6/drivers/scsi/dc390.h Sat May 22 19:47:24 2004 +++ linux-2.6.6-bk6-tmscsim/drivers/scsi/dc390.h Sat May 22 00:05:08 2004 @@ -33,7 +33,6 @@ # define USE_NEW_EH #endif -static int DC390_detect(Scsi_Host_Template *psht); static int DC390_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *)); static int DC390_abort(Scsi_Cmnd *cmd); static int DC390_reset(Scsi_Cmnd *cmd); diff -ur --exclude=CVS vanilla/linux-2.6.6/drivers/scsi/scsiiom.c linux-2.6.6-bk6-tmscsim/drivers/scsi/scsiiom.c --- vanilla/linux-2.6.6/drivers/scsi/scsiiom.c Sat May 22 19:47:24 2004 +++ linux-2.6.6-bk6-tmscsim/drivers/scsi/scsiiom.c Sun May 23 22:03:50 2004 @@ -789,7 +789,7 @@ /* read and eval received messages */ -void +static void dc390_MsgIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus) { PDCB pDCB = pACB->pActiveDCB; @@ -948,7 +948,7 @@ dc390_DataIO_Comm (pACB, pSRB, READ_DIRECTION); } -void +static void dc390_CommandPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus) { PDCB pDCB; @@ -989,7 +989,7 @@ //DC390_write8 (DMA_Cmd, DMA_IDLE_CMD); } -void +static void dc390_MsgOutPhase( PACB pACB, PSRB pSRB, PUCHAR psstatus) { UCHAR bval, i, cnt; @@ -1097,7 +1097,7 @@ } -void +static void dc390_Disconnect( PACB pACB ) { PDCB pDCB; @@ -1179,7 +1179,7 @@ } -void +static void dc390_Reselect( PACB pACB ) { PDCB pDCB; @@ -1349,10 +1349,10 @@ }; -void +static void dc390_SRBdone( PACB pACB, PDCB pDCB, PSRB pSRB ) { - UCHAR bval, status, i, DCB_removed; + UCHAR bval, status, i; PSCSICMD pcmd; PSCSI_INQDATA ptr; PSGL ptr2; @@ -1362,7 +1362,6 @@ /* KG: Moved pci_unmap here */ dc390_pci_unmap(pSRB); - DCB_removed = 0; status = pSRB->TargetStatus; ptr = (PSCSI_INQDATA) (pcmd->request_buffer); if( pcmd->use_sg ) @@ -1569,8 +1568,6 @@ (pcmd->sense_buffer[2] & 0xf) == ILLEGAL_REQUEST) || host_byte(pcmd->result) & DID_ERROR ) { /* device not present: remove */ - //dc390_Going_remove (pDCB, pSRB); - dc390_remove_dev (pACB, pDCB); DCB_removed = 1; if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && ((pcmd->device->lun == 0) || (pcmd->device->lun == pACB->pScsiHost->max_lun - 1)) ) @@ -1582,24 +1579,14 @@ if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && (pcmd->device->lun == pACB->pScsiHost->max_lun - 1) ) pACB->scan_devices = END_SCAN ; - /* pACB->DeviceCnt++; */ /* Dev is added on INQUIRY */ } } } - - //if( pSRB->pcmd->cmnd[0] == INQUIRY && - // (host_byte(pcmd->result) == DID_OK || status_byte(pcmd->result) & CHECK_CONDITION) ) + if( pcmd->cmnd[0] == INQUIRY && (pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) ) { - if ((ptr->DevType & SCSI_DEVTYPE) == TYPE_NODEV && !DCB_removed) - { - //printk ("DC390: Type = nodev! (%02i-%i)\n", pcmd->target, pcmd->lun); - /* device not present: remove */ - //dc390_Going_remove (pDCB, pSRB); - dc390_remove_dev (pACB, pDCB); DCB_removed = 1; - } - else + if ((ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV) { /* device found: add */ dc390_add_dev (pACB, pDCB, ptr); @@ -1608,11 +1595,11 @@ if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && (pcmd->device->lun == pACB->pScsiHost->max_lun - 1) ) pACB->scan_devices = 0; - }; + } pcmd->resid = pcmd->request_bufflen - pSRB->TotalXferredLen; - if (!DCB_removed) dc390_Going_remove (pDCB, pSRB); + dc390_Going_remove (pDCB, pSRB); /* Add to free list */ dc390_Free_insert (pACB, pSRB); @@ -1625,7 +1612,7 @@ /* Remove all SRBs from Going list and inform midlevel */ -void +static void dc390_DoingSRB_Done( PACB pACB, PSCSICMD cmd ) { PDCB pDCB, pdcb; @@ -1760,4 +1747,3 @@ if( pACB->pActiveDCB->pActiveSRB->SRBState & (SRB_START_+SRB_MSGOUT) ) DC390_write8 (ScsiCmd, CLEAR_FIFO_CMD); } - diff -ur --exclude=CVS vanilla/linux-2.6.6/drivers/scsi/tmscsim.c linux-2.6.6-bk6-tmscsim/drivers/scsi/tmscsim.c --- vanilla/linux-2.6.6/drivers/scsi/tmscsim.c Sat May 22 19:47:24 2004 +++ linux-2.6.6-bk6-tmscsim/drivers/scsi/tmscsim.c Sun May 23 23:11:35 2004 @@ -267,7 +267,6 @@ #include <linux/init.h> #include <linux/spinlock.h> -#if defined(MODULE) static struct pci_device_id tmscsim_pci_tbl[] = { { .vendor = PCI_VENDOR_ID_AMD, @@ -278,8 +277,7 @@ { } /* Terminating entry */ }; MODULE_DEVICE_TABLE(pci, tmscsim_pci_tbl); -#endif - + #define USE_SPINLOCKS 1 #define DC390_IFLAGS unsigned long iflags @@ -342,6 +340,8 @@ static int DC390_release(struct Scsi_Host *host); static int dc390_shutdown (struct Scsi_Host *host); +static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start, + off_t offset, int length, int inout); static PACB dc390_pACB_start= NULL; static PACB dc390_pACB_current = NULL; @@ -351,16 +351,14 @@ /* Startup values, to be overriden on the commandline */ static int tmscsim[] = {-2, -2, -2, -2, -2, -2}; +static int tmscsim_paramnum = ARRAY_SIZE(tmscsim); -#if defined(MODULE) -MODULE_PARM(tmscsim, "1-6i"); +module_param_array(tmscsim, int, tmscsim_paramnum, 0); MODULE_PARM_DESC(tmscsim, "Host SCSI ID, Speed (0=10MHz), Device Flags, Adapter Flags, Max Tags (log2(tags)-1), DelayReset (s)"); MODULE_AUTHOR("C.L. Huang / Kurt Garloff"); MODULE_DESCRIPTION("SCSI host adapter driver for Tekram DC390 and other AMD53C974A based PCI SCSI adapters"); MODULE_LICENSE("GPL"); - MODULE_SUPPORTED_DEVICE("sd,sr,sg,st"); -#endif static PVOID dc390_phase0[]={ dc390_DataOut_0, @@ -1051,41 +1049,29 @@ * commands and alloc a DCB for the device if not yet there. DCB will * be removed in dc390_SRBdone if SEL_TIMEOUT */ - if( (pACB->scan_devices == END_SCAN) && (cmd->cmnd[0] != INQUIRY) ) - pACB->scan_devices = 0; - - else if( (pACB->scan_devices) && (cmd->cmnd[0] == READ_6) ) + if (((pACB->scan_devices == END_SCAN) && (cmd->cmnd[0] != INQUIRY)) || + ((pACB->scan_devices) && (cmd->cmnd[0] == READ_6))) pACB->scan_devices = 0; - if( (pACB->scan_devices || cmd->cmnd[0] == TEST_UNIT_READY || cmd->cmnd[0] == INQUIRY) && - !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun)) ) - { - pACB->scan_devices = 1; - - dc390_initDCB( pACB, &pDCB, cmd->device->id, cmd->device->lun ); - if (!pDCB) - { - printk (KERN_ERR "DC390: kmalloc for DCB failed, target %02x lun %02x\n", - cmd->device->id, cmd->device->lun); - goto fail; - } - - } - else if( !(pACB->scan_devices) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun)) ) - { + if ((pACB->scan_devices || cmd->cmnd[0] == TEST_UNIT_READY || cmd->cmnd[0] == INQUIRY) && + !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) { + pACB->scan_devices = 1; + DCBDEBUG(printk("Scanning target %02x lun %02x\n", cmd->device->id, cmd->device->lun)); + } else if (!(pACB->scan_devices) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) { printk(KERN_INFO "DC390: Ignore target %02x lun %02x\n", cmd->device->id, cmd->device->lun); goto fail; } - else - { - pDCB = dc390_findDCB (pACB, cmd->device->id, cmd->device->lun); - if (!pDCB) - { /* should never happen */ - printk (KERN_ERR "DC390: no DCB failed, target %02x lun %02x\n", - cmd->device->id, cmd->device->lun); - goto fail; - } + + pDCB = dc390_findDCB (pACB, cmd->device->id, cmd->device->lun); + + /* Should it be: BUG_ON(!pDCB); ? */ + + if (!pDCB) + { /* should never happen */ + printk (KERN_ERR "DC390: no DCB found, target %02x lun %02x\n", + cmd->device->id, cmd->device->lun); + goto fail; } pACB->Cmds++; @@ -1856,7 +1842,7 @@ * * Inputs : host - pointer to this host adapter's structure * io_port - IO ports mapped to this adapter - * Irq - IRQ assigned to this adpater + * irq - IRQ assigned to this adpater * struct pci_dev - PCI access handle * index - Adapter index * @@ -1865,10 +1851,8 @@ * Note: written in capitals, because the locking is only done here, * not in DC390_detect, called from outside ***********************************************************************/ - -static int __init DC390_init (PSHT psht, ULONG io_port, UCHAR Irq, struct pci_dev *pdev, UCHAR index) +static int __init dc390_init (PSH psh, unsigned long io_port, u8 irq, struct pci_dev *pdev, UCHAR index) { - PSH psh; PACB pACB; if (dc390_CheckEEpromCheckSum (PDEV, index)) @@ -1890,25 +1874,16 @@ dc390_check_for_safe_settings (); dc390_EEprom_Override (index); } - - psh = scsi_register( psht, sizeof(DC390_ACB) ); - if( !psh ) return( -1 ); - - scsi_set_device(psh, &pdev->dev); pACB = (PACB) psh->hostdata; DEBUG0(printk(KERN_INFO "DC390: pSH = %8x, Index %02i\n", (UINT) psh, index)); - dc390_initACB( psh, io_port, Irq, index ); + dc390_initACB( psh, io_port, irq, index ); PDEVSET; - if( !dc390_initAdapter( psh, io_port, Irq, index ) ) + if( !dc390_initAdapter( psh, io_port, irq, index ) ) { - DEBUG0(printk("DC390: pACB = %8x, pDCBmap = %8x, pSRB_array = %8x\n",\ - (UINT) pACB, (UINT) pACB->DCBmap, (UINT) pACB->SRB_array)); - DEBUG0(printk("DC390: ACB size= %4x, DCB size= %4x, SRB size= %4x\n",\ - sizeof(DC390_ACB), sizeof(DC390_DCB), sizeof(DC390_SRB) )); return (0); } else @@ -1927,42 +1902,119 @@ PCI_WRITE_CONFIG_WORD (PDEV, PCI_STATUS, (PCI_STATUS_SIG_SYSTEM_ERROR | PCI_STATUS_DETECTED_PARITY)); } -int __init DC390_detect (Scsi_Host_Template *psht) +/** + * dc390_slave_alloc - Called by the scsi mid layer to tell us about a new + * scsi device that we need to deal with. + * + * @scsi_device: The new scsi device that we need to handle. + */ +static int dc390_slave_alloc(struct scsi_device *scsi_device) { - struct pci_dev *pdev = NULL; - UCHAR irq; - ULONG io_port; - - dc390_pACB_start = NULL; + PDCB pDCB; + PACB pACB = (PACB) scsi_device->host->hostdata; + dc390_initDCB(pACB, &pDCB, scsi_device->id, scsi_device->lun); + if (pDCB != NULL) + return 0; + return -ENOMEM; +} - if ( PCI_PRESENT ) - while ((pdev = pci_find_device(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD53C974, pdev))) - { - if (pci_enable_device (pdev)) - continue; +/** + * dc390_slave_destroy - Called by the scsi mid layer to tell us about a + * device that is going away. + * + * @scsi_device: The scsi device that we need to remove. + */ +static void dc390_slave_destroy(struct scsi_device *scsi_device) +{ + PACB pACB = (PACB) scsi_device->host->hostdata; + PDCB pDCB = dc390_findDCB (pACB, scsi_device->id, scsi_device->lun);; + if (pDCB != NULL) + dc390_remove_dev(pACB, pDCB); + else + printk(KERN_ERR"%s() called for non-existing device!\n", __FUNCTION__); +} + +static Scsi_Host_Template driver_template = { + .module = THIS_MODULE, + .proc_name = "tmscsim", + .proc_info = DC390_proc_info, + .name = DC390_BANNER " V" DC390_VERSION, + .slave_alloc = dc390_slave_alloc, + .slave_destroy = dc390_slave_destroy, + .queuecommand = DC390_queue_command, + .eh_abort_handler = DC390_abort, + .eh_bus_reset_handler = DC390_reset, + .bios_param = DC390_bios_param, + .can_queue = 42, + .this_id = 7, + .sg_tablesize = SG_ALL, + .cmd_per_lun = 16, + .use_clustering = DISABLE_CLUSTERING, +}; - if (pci_set_dma_mask(pdev, 0xffffffff)) { - printk(KERN_ERR "DC390(%i): No suitable DMA available.\n", dc390_adapterCnt); - continue; - } - PCI_GET_IO_AND_IRQ; - DEBUG0(printk(KERN_INFO "DC390(%i): IO_PORT=%04x,IRQ=%x\n", dc390_adapterCnt, (UINT) io_port, irq)); +static int __devinit dc390_init_one(struct pci_dev *dev, + const struct pci_device_id *id) +{ + struct Scsi_Host *scsi_host; + unsigned long io_port; + u8 irq; + PACB pACB; + int ret = -ENOMEM; + + if (pci_enable_device(dev)) + return -ENODEV; + + io_port = pci_resource_start(dev, 0); + irq = dev->irq; + + /* allocate scsi host information (includes out adapter) */ + scsi_host = scsi_host_alloc(&driver_template, sizeof(struct _ACB)); + if (!scsi_host) + goto nomem; + + pACB = (PACB)scsi_host->hostdata; + + if (dc390_init(scsi_host, io_port, irq, dev, dc390_adapterCnt)) { + ret = -EBUSY; + goto busy; + } - if( !DC390_init(psht, io_port, irq, PDEV, dc390_adapterCnt)) - { - pci_set_master(pdev); - dc390_set_pci_cfg (PDEV); - dc390_adapterCnt++; - } + pci_set_master(dev); + dc390_set_pci_cfg(dev); + dc390_adapterCnt++; + + /* get the scsi mid level to scan for new devices on the bus */ + if (scsi_add_host(scsi_host, &dev->dev)) { + ret = -ENODEV; + goto nodev; } - else - printk (KERN_ERR "DC390: No PCI BIOS found!\n"); - - if (dc390_adapterCnt) - psht->proc_name = "tmscsim"; + pci_set_drvdata(dev, scsi_host); + scsi_scan_host(scsi_host); - printk(KERN_INFO "DC390: %i adapters found\n", dc390_adapterCnt); - return( dc390_adapterCnt ); + return 0; + +nodev: +busy: + scsi_host_put(scsi_host); +nomem: + pci_disable_device(dev); + return ret; +} + +/** + * dc390_remove_one - Called to remove a single instance of the adapter. + * + * @dev: The PCI device to remove. + */ +static void __devexit dc390_remove_one(struct pci_dev *dev) +{ + struct Scsi_Host *scsi_host = pci_get_drvdata(dev); + + scsi_remove_host(scsi_host); + DC390_release(scsi_host); + pci_disable_device(dev); + scsi_host_put(scsi_host); + pci_set_drvdata(dev, NULL); } /******************************************************************** @@ -2177,24 +2229,25 @@ release_region(host->io_port,host->n_io_port); dc390_freeDCBs (host); DC390_UNLOCK_IO(host); - scsi_unregister(host); return( 1 ); } -static Scsi_Host_Template driver_template = { - .proc_name = "tmscsim", - .proc_info = DC390_proc_info, - .name = DC390_BANNER " V" DC390_VERSION, - .detect = DC390_detect, - .release = DC390_release, - .queuecommand = DC390_queue_command, - .eh_abort_handler = DC390_abort, - .eh_bus_reset_handler = DC390_reset, - .bios_param = DC390_bios_param, - .can_queue = 42, - .this_id = 7, - .sg_tablesize = SG_ALL, - .cmd_per_lun = 16, - .use_clustering = DISABLE_CLUSTERING, +static struct pci_driver dc390_driver = { + .name = "tmscsim", + .id_table = tmscsim_pci_tbl, + .probe = dc390_init_one, + .remove = __devexit_p(dc390_remove_one), }; -#include "scsi_module.c" + +static int __init dc390_module_init(void) +{ + return pci_module_init(&dc390_driver); +} + +static void __exit dc390_module_exit(void) +{ + pci_unregister_driver(&dc390_driver); +} + +module_init(dc390_module_init); +module_exit(dc390_module_exit); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-23 21:19 ` Guennadi Liakhovetski @ 2004-05-26 11:45 ` Christoph Hellwig 2004-05-26 19:37 ` [PATCH] fix comment in scsi_host.h Guennadi Liakhovetski ` (2 more replies) 0 siblings, 3 replies; 35+ messages in thread From: Christoph Hellwig @ 2004-05-26 11:45 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list On Sun, May 23, 2004 at 11:19:58PM +0200, Guennadi Liakhovetski wrote: > > Looks mostly goo to me, one thing I worry about is the complete > > removal of the scan_device flag, I don't fully understand what it's > > supposed to do, but we should be able to always set in ->slave_alloc > > and then clear it in ->slave_configure when we know probing is over > > for this device. > > Right, thanks. I didn't understand at all what scan_devices was doing > there, so, I tried to preserve its semantics in the driver, and "just" > overlooked its setting in dc390_SRBdone:-( But, wouldn't it be safer to > just restore the relevant parts there, rather than moving it in > slave_alloc / configure? Because... emn, well, I wanted to write "because > it is tested later on in dc390_SRBdone: > if (pACB->scan_devices) pACB->DeviceCnt++; > ", but then I checked, where else this DeviceCnt is used, and found only > initialisation to 0, the increment above, no decrement, and a proc-output: > > SPRINTF("Nr of attached devices: %i, Nr of DCBs: %i\n", pACB->DeviceCnt, pACB->DCBCnt); > > Whereas, DCBCnt is incremented on slave_alloc, decremented on > slave_destroy and used in a couple more places. So, is it safe to assume > that DeviceCnt is redundant and remove it in a next patch?... Then it > might be already safe to move scan_devices as you suggest? Looks like a way to go. I'm not sure what scan_device is supposed to do as I already wrote, but from looking at when it's set/cleared I'm pretty sure it tries to indicate whethere we're currently scanning for devices. And useing slave_alloc/slave_configure is a much safer way to find that out then guessing from the commands sent. > > In dc390_init_one you don't call pci_disable_device on failure, > > best switch to goto for a single shared error path to avoid all > > these leaks. > > Done. Actually, I copied this (and other) new function(s) from dc395x, so, > they have this bug too... There's quite a lot drivers missing it, because it's not nessecary on most PCs. > And, I asume, it has to be done in remove_one too, right? Yes. > RCS file: /usr/src/cvs/linux-2_6/include/scsi/scsi_host.h,v > retrieving revision 1.1.1.3 > diff -u -r1.1.1.3 scsi_host.h > --- include/scsi/scsi_host.h 19 May 2004 16:08:49 -0000 1.1.1.3 > +++ include/scsi/scsi_host.h 23 May 2004 20:18:31 -0000 > @@ -151,7 +151,7 @@ > * here then you will get a call to slave_configure(), then the > * device will be used for however long it is kept around, then when > * the device is removed from the system (or * possibly at reboot > - * time), you will then get a call to slave_detach(). This is > + * time), you will then get a call to slave_destroy(). This is > * assuming you implement slave_configure and slave_destroy. > * However, if you allocate memory and hang it off the device struct, > * then you must implement the slave_destroy() routine at a minimum > @@ -185,7 +185,7 @@ > * specific setup basis... > * 6. Return 0 on success, non-0 on error. The device will be marked > * as offline on error so that no access will occur. If you return > - * non-0, your slave_detach routine will never get called for this > + * non-0, your slave_destroy routine will never get called for this This looks okay, but can you please send it to the list as a separate patch? Rest of the patch also looks go to go in as-is, the additional items can be fixed in followup-patches. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fix comment in scsi_host.h 2004-05-26 11:45 ` Christoph Hellwig @ 2004-05-26 19:37 ` Guennadi Liakhovetski 2004-05-26 21:07 ` [PATCH] tmscsim: new interfaces Guennadi Liakhovetski 2004-05-27 21:20 ` Guennadi Liakhovetski 2 siblings, 0 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-26 19:37 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, Linux SCSI list [-- Attachment #1: Type: TEXT/PLAIN, Size: 293 bytes --] On Wed, 26 May 2004, Christoph Hellwig wrote: > On Sun, May 23, 2004 at 11:19:58PM +0200, Guennadi Liakhovetski wrote: > > diff -u -r1.1.1.3 scsi_host.h ... > This looks okay, but can you please send it to the list as a separate patch? Attached. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 1221 bytes --] diff -u a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h --- a/include/scsi/scsi_host.h 19 May 2004 16:08:49 +++ b/include/scsi/scsi_host.h 23 May 2004 20:18:31 @@ -151,7 +151,7 @@ * here then you will get a call to slave_configure(), then the * device will be used for however long it is kept around, then when * the device is removed from the system (or * possibly at reboot - * time), you will then get a call to slave_detach(). This is + * time), you will then get a call to slave_destroy(). This is * assuming you implement slave_configure and slave_destroy. * However, if you allocate memory and hang it off the device struct, * then you must implement the slave_destroy() routine at a minimum @@ -185,7 +185,7 @@ * specific setup basis... * 6. Return 0 on success, non-0 on error. The device will be marked * as offline on error so that no access will occur. If you return - * non-0, your slave_detach routine will never get called for this + * non-0, your slave_destroy routine will never get called for this * device, so don't leave any loose memory hanging around, clean * up after yourself before returning non-0 * ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-26 11:45 ` Christoph Hellwig 2004-05-26 19:37 ` [PATCH] fix comment in scsi_host.h Guennadi Liakhovetski @ 2004-05-26 21:07 ` Guennadi Liakhovetski 2004-05-27 21:20 ` Guennadi Liakhovetski 2 siblings, 0 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-26 21:07 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, Linux SCSI list [-- Attachment #1: Type: TEXT/PLAIN, Size: 840 bytes --] On Wed, 26 May 2004, Christoph Hellwig wrote: > On Sun, May 23, 2004 at 11:19:58PM +0200, Guennadi Liakhovetski wrote: > > > > Whereas, DCBCnt is incremented on slave_alloc, decremented on > > slave_destroy and used in a couple more places. So, is it safe to assume > > that DeviceCnt is redundant and remove it in a next patch?... Then it > > might be already safe to move scan_devices as you suggest? > > Looks like a way to go. I'm not sure what scan_device is supposed to > do as I already wrote, but from looking at when it's set/cleared I'm > pretty sure it tries to indicate whethere we're currently scanning for > devices. And useing slave_alloc/slave_configure is a much safer way to > find that out then guessing from the commands sent. So, here's the first one - remove DeviceCnt. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 2750 bytes --] diff -u a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c --- a/drivers/scsi/scsiiom.c 26 May 2004 19:49:00 +++ b/drivers/scsi/scsiiom.c 26 May 2004 20:23:32 @@ -1302,7 +1302,6 @@ if (pDCB == pACB->pDCBRunRobin) pACB->pDCBRunRobin = pDCB->pNextDCB; kfree (pDCB); pACB->DCBCnt--; - /* pACB->DeviceCnt--; */ }; @@ -1590,7 +1589,6 @@ { /* device found: add */ dc390_add_dev (pACB, pDCB, ptr); - if (pACB->scan_devices) pACB->DeviceCnt++; } if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && (pcmd->device->lun == pACB->pScsiHost->max_lun - 1) ) diff -u a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c --- a/drivers/scsi/tmscsim.c 26 May 2004 20:26:21 +++ b/drivers/scsi/tmscsim.c 26 May 2004 20:24:01 @@ -1734,7 +1734,6 @@ pACB->AdapterIndex = index; pACB->status = 0; psh->this_id = dc390_eepromBuf[index][EE_ADAPT_SCSI_ID]; - pACB->DeviceCnt = 0; pACB->DCBCnt = 0; pACB->TagMaxNum = 2 << dc390_eepromBuf[index][EE_TAG_CMD_NUM]; pACB->ACBFlag = 0; @@ -2089,7 +2088,7 @@ SPRINTF(" Lost arbitrations %i, Sel. connected %i, Connected: %s\n", pACB->SelLost, pACB->SelConn, pACB->Connected? "Yes": "No"); - SPRINTF("Nr of attached devices: %i, Nr of DCBs: %i\n", pACB->DeviceCnt, pACB->DCBCnt); + SPRINTF("Nr of DCBs: %i\n", pACB->DCBCnt); SPRINTF("Map of attached LUNs: %02x %02x %02x %02x %02x %02x %02x %02x\n", pACB->DCBmap[0], pACB->DCBmap[1], pACB->DCBmap[2], pACB->DCBmap[3], pACB->DCBmap[4], pACB->DCBmap[5], pACB->DCBmap[6], pACB->DCBmap[7]); diff -u a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h --- a/drivers/scsi/tmscsim.h 24 May 2004 19:22:02 +++ b/drivers/scsi/tmscsim.h 26 May 2004 20:23:14 @@ -196,10 +196,8 @@ UCHAR SRBCount; UCHAR AdapterIndex; /*; nth Adapter this driver */ -UCHAR DeviceCnt; UCHAR DCBCnt; -/* 0x10: */ UCHAR TagMaxNum; UCHAR ACBFlag; UCHAR Gmode2; @@ -213,13 +211,11 @@ PSRB pFreeSRB; PSRB pTmpSRB; -/* 0x2c: */ UCHAR msgin123[4]; UCHAR DCBmap[MAX_SCSI_ID]; UCHAR Connected; UCHAR pad; -/* 0x30: */ #if defined(USE_SPINLOCKS) && USE_SPINLOCKS > 1 && (defined(CONFIG_SMP) || DEBUG_SPINLOCKS > 0) spinlock_t lock; #endif @@ -230,20 +226,17 @@ UCHAR Ignore_IRQ; /* Not used */ PDEVDECL1; /* Pointer to PCI cfg. space */ -/* 0x40/0x3c: */ + ULONG Cmds; UINT SelLost; UINT SelConn; UINT CmdInQ; UINT CmdOutOfSRB; -/* 0x54/0x50: */ struct timer_list Waiting_Timer; -/* 0x68/0x64: */ + DC390_SRB TmpSRB; -/* 0xcc/0xc8: */ DC390_SRB SRB_array[MAX_SRB_CNT]; /* 50 SRBs */ -/* 0xfa4/0xfa0: */ }; typedef struct _ACB DC390_ACB, *PACB; ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-26 11:45 ` Christoph Hellwig 2004-05-26 19:37 ` [PATCH] fix comment in scsi_host.h Guennadi Liakhovetski 2004-05-26 21:07 ` [PATCH] tmscsim: new interfaces Guennadi Liakhovetski @ 2004-05-27 21:20 ` Guennadi Liakhovetski 2004-05-28 13:00 ` Christoph Hellwig 2004-05-29 15:36 ` James Bottomley 2 siblings, 2 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-27 21:20 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list [-- Attachment #1: Type: TEXT/PLAIN, Size: 968 bytes --] On Wed, 26 May 2004, Christoph Hellwig wrote: > Looks like a way to go. I'm not sure what scan_device is supposed to > do as I already wrote, but from looking at when it's set/cleared I'm > pretty sure it tries to indicate whethere we're currently scanning for > devices. And useing slave_alloc/slave_configure is a much safer way to > find that out then guessing from the commands sent. Attached is a patch, that moves scan_device to slave_alloc/_configure/_destroy, as you suggested (to be precise, as I understood your suggestion:-)). Please consider. The patch also contains some more clean-up. Don't know, if you, James, would prefer it as a separate patch, or left alone altogether. Those minor details are disturbing the eye, but are, perhaps, not important enough to bother you with a separate patch:-) But if you want it separate - no problem, will do that. As usual, tested on my AM53C974. Please, consider. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 10558 bytes --] diff -ur --exclude=CVS a/drivers/scsi/dc390.h b/drivers/scsi/dc390.h --- a/drivers/scsi/dc390.h Wed May 26 22:25:38 2004 +++ b/drivers/scsi/dc390.h Thu May 27 22:57:23 2004 @@ -14,12 +14,9 @@ #define DC390_H #include <linux/version.h> -#ifndef KERNEL_VERSION -# define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c)) -#endif #define DC390_BANNER "Tekram DC390/AM53C974" -#define DC390_VERSION "2.1c 2004-05-23" +#define DC390_VERSION "2.1d 2004-05-27" /* We don't have eh_abort_handler, eh_device_reset_handler, * eh_bus_reset_handler, eh_host_reset_handler yet! @@ -32,13 +29,4 @@ # define NEW_EH use_new_eh_code: 1, # define USE_NEW_EH #endif - -static int DC390_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *)); -static int DC390_abort(Scsi_Cmnd *cmd); -static int DC390_reset(Scsi_Cmnd *cmd); -static int DC390_bios_param(struct scsi_device *sdev, struct block_device *dev, - sector_t capacity, int geom[]); - -static int DC390_release(struct Scsi_Host *); - #endif /* DC390_H */ diff -ur --exclude=CVS a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c --- a/drivers/scsi/scsiiom.c Thu May 27 22:37:18 2004 +++ b/drivers/scsi/scsiiom.c Thu May 27 22:32:23 2004 @@ -12,7 +12,7 @@ pDCB->TagMask &= ~(1 << pSRB->TagNumber); /* free tag mask */ pSRB->TagNumber = 255; } -}; +} static UCHAR @@ -75,7 +75,7 @@ printk (KERN_WARNING "DC390: Out of tags for Dev. %02x %02x\n", pDCB->TargetID, pDCB->TargetLUN); return 1; //goto no_tag; - }; + } DC390_write8 (ScsiFifo, SIMPLE_QUEUE_TAG); pDCB->TagMask |= (1 << tag_no); pSRB->TagNumber = tag_no; DC390_write8 (ScsiFifo, tag_no); @@ -86,7 +86,7 @@ { // no_tag: DEBUG1(printk (KERN_DEBUG "DC390: Select w%s/DisCn for Cmd %li (SRB %p), No TagQ\n", (disc_allowed?"":"o"), pSRB->pcmd->pid, pSRB)); - }; + } pSRB->SRBState = SRB_START_; @@ -104,7 +104,7 @@ //pSRB->SRBState = SRB_MSGOUT_; pSRB->SRBState |= DO_SYNC_NEGO; cmd = SEL_W_ATN_STOP; - }; + } /* Command is written in CommandPhase, if SEL_W_ATN_STOP ... */ if (cmd != SEL_W_ATN_STOP) @@ -125,7 +125,7 @@ ptr = (PUCHAR) pSRB->pcmd->cmnd; for (i=0; i<pSRB->pcmd->cmd_len; i++) DC390_write8 (ScsiFifo, *(ptr++)); - }; + } } DEBUG0(if (pACB->pActiveDCB) \ printk (KERN_WARNING "DC390: ActiveDCB != 0\n")); @@ -141,7 +141,7 @@ //DC390_write8 (ScsiCmd, CLEAR_FIFO_CMD); pACB->SelLost++; return 1; - }; + } DC390_write8 (ScsiCmd, cmd); pACB->pActiveDCB = pDCB; pDCB->pActiveSRB = pSRB; pACB->Connected = 1; @@ -176,7 +176,7 @@ { printk (KERN_ERR "DC390: DMA error (%02x)!\n", dstate); return dstate; - }; + } if (dstate & DMA_XFER_DONE) { UINT residual, xferCnt; int ctr = 6000000; @@ -253,7 +253,7 @@ { DEBUG0(printk (KERN_WARNING "DC390 Int w/o SCSI actions (only DMA?)\n")); return IRQ_NONE; - }; + } #else //DC390_write32 (DMA_ScsiBusCtrl, WRT_ERASE_DMA_STAT | EN_INT_ON_PCI_ABORT); //dstatus = DC390_read8 (DMA_Status); @@ -313,7 +313,7 @@ { printk (KERN_ERR "DC390: Suc. op/ Serv. req: pActiveDCB = 0!\n"); goto unlock; - }; + } pSRB = pDCB->pActiveSRB; if( pDCB->DCBFlag & ABORT_DEV_ ) dc390_EnableMsgOut_Abort (pACB, pSRB); @@ -549,7 +549,7 @@ DC390_write8 (CtrlReg3, pDCB->CtrlR3); DC390_write8 (CtrlReg4, pDCB->CtrlR4); dc390_SetXferRate (pACB, pDCB); -}; +} #ifdef DC390_DEBUG0 @@ -561,7 +561,7 @@ for (i = 1; i < len; i++) printk (" %02x", MsgBuf[i]); printk ("\n"); -}; +} #endif #define DC390_ENABLE_MSGOUT DC390_write8 (ScsiCmd, SET_ATN_CMD) @@ -671,11 +671,11 @@ { printk (KERN_INFO "DC390: Set sync nego period to %ins\n", pDCB->NegoPeriod << 2); pSRB->MsgInBuf[3] = pDCB->NegoPeriod; - }; + } memcpy (pSRB->MsgOutBuf, pSRB->MsgInBuf, 5); pSRB->MsgCnt = 5; DC390_ENABLE_MSGOUT; - }; + } pSRB->SRBState &= ~DO_SYNC_NEGO; pDCB->SyncMode |= SYNC_ENABLE+SYNC_NEGO_DONE; @@ -713,7 +713,7 @@ } dc390_reprog (pACB, pDCB); -}; +} /* handle RESTORE_PTR */ @@ -761,7 +761,7 @@ } pSRB->TotalXferredLen = pSRB->Saved_Ptr; -}; +} /* According to the docs, the AM53C974 reads the message and @@ -832,7 +832,7 @@ dc390_MsgIn_set_async (pACB, pSRB); else dc390_MsgIn_set_sync (pACB, pSRB); - }; + } // nothing has to be done case COMMAND_COMPLETE: break; @@ -1276,7 +1276,7 @@ DCBDEBUG(printk (KERN_INFO "DC390: Driver won't free DCB (ID %i, LUN %i): 0x%08x because of SRBCnt %i\n",\ pDCB->TargetID, pDCB->TargetLUN, (int)pDCB, pDCB->GoingSRBCnt)); return; - }; + } pACB->DCBmap[pDCB->TargetID] &= ~(1 << pDCB->TargetLUN); // The first one @@ -1302,7 +1302,7 @@ if (pDCB == pACB->pDCBRunRobin) pACB->pDCBRunRobin = pDCB->pNextDCB; kfree (pDCB); pACB->DCBCnt--; -}; +} static UCHAR __inline__ @@ -1313,7 +1313,7 @@ if (memcmp (name, dc390_baddevname1[i], 28) == 0) return 1; return 0; -}; +} static void @@ -1335,7 +1335,7 @@ else pDCB->MaxCommand = 1; } -}; +} static void @@ -1345,7 +1345,7 @@ pDCB->DevType = bval1; /* if (bval1 == TYPE_DISK || bval1 == TYPE_MOD) */ dc390_disc_tagq_set (pDCB, ptr); -}; +} static void @@ -1562,23 +1562,6 @@ pcmd->sense_buffer[2], pcmd->sense_buffer[3]); else printk ("\n"); #endif - if( (host_byte(pcmd->result) != DID_OK && !(status_byte(pcmd->result) & CHECK_CONDITION) && !(status_byte(pcmd->result) & BUSY)) || - ((driver_byte(pcmd->result) & DRIVER_SENSE) && (pcmd->sense_buffer[0] & 0x70) == 0x70 && - (pcmd->sense_buffer[2] & 0xf) == ILLEGAL_REQUEST) || host_byte(pcmd->result) & DID_ERROR ) - { - /* device not present: remove */ - - if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && - ((pcmd->device->lun == 0) || (pcmd->device->lun == pACB->pScsiHost->max_lun - 1)) ) - pACB->scan_devices = 0; - } - else - { - /* device present: add */ - if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && - (pcmd->device->lun == pACB->pScsiHost->max_lun - 1) ) - pACB->scan_devices = END_SCAN ; - } } } @@ -1590,9 +1573,6 @@ /* device found: add */ dc390_add_dev (pACB, pDCB, ptr); } - if( (pcmd->device->id == pACB->pScsiHost->max_id - 1) && - (pcmd->device->lun == pACB->pScsiHost->max_lun - 1) ) - pACB->scan_devices = 0; } pcmd->resid = pcmd->request_bufflen - pSRB->TotalXferredLen; diff -ur --exclude=CVS a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c --- a/drivers/scsi/tmscsim.c Thu May 27 22:37:18 2004 +++ b/drivers/scsi/tmscsim.c Thu May 27 22:57:12 2004 @@ -174,6 +174,9 @@ * 2.1b2 04/02/01 CH (applied 05.04) Fix error-handling * * 2.1c 04/05/23 GL Update to use the new pci_driver API, * * some scsi EH updates, more cleanup. * + * 2.1d 04/05/27 GL Moved setting of scan_devices to * + * slave_alloc/_configure/_destroy, as * + * suggested by CH. * ***********************************************************************/ /* Uncomment SA_INTERRUPT, if the driver refuses to share its IRQ with other devices */ @@ -1031,8 +1034,8 @@ * 2.0.x: always return 0 * 2.1.x: old model: (use_new_eh_code == 0): like 2.0.x * TO BE DONE: - * new model: return 0 if successful - * return 1 if command cannot be queued (queue full) + * new model: return 0 if successful, or must not be re-queued + * return 1 if command cannot be queued (queue full) * command will be inserted in midlevel queue then ... * ***********************************************************************/ @@ -1050,16 +1053,7 @@ /* TODO: Change the policy: Always accept TEST_UNIT_READY or INQUIRY * commands and alloc a DCB for the device if not yet there. DCB will * be removed in dc390_SRBdone if SEL_TIMEOUT */ - - if (((pACB->scan_devices == END_SCAN) && (cmd->cmnd[0] != INQUIRY)) || - ((pACB->scan_devices) && (cmd->cmnd[0] == READ_6))) - pACB->scan_devices = 0; - - if ((pACB->scan_devices || cmd->cmnd[0] == TEST_UNIT_READY || cmd->cmnd[0] == INQUIRY) && - !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) { - pACB->scan_devices = 1; - DCBDEBUG(printk("Scanning target %02x lun %02x\n", cmd->device->id, cmd->device->lun)); - } else if (!(pACB->scan_devices) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) { + if (!(pACB->scan_devices) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) { printk(KERN_INFO "DC390: Ignore target %02x lun %02x\n", cmd->device->id, cmd->device->lun); goto fail; @@ -1914,8 +1908,10 @@ PDCB pDCB; PACB pACB = (PACB) scsi_device->host->hostdata; dc390_initDCB(pACB, &pDCB, scsi_device->id, scsi_device->lun); - if (pDCB != NULL) + if (pDCB != NULL) { + pACB->scan_devices = 1; return 0; + } return -ENOMEM; } @@ -1929,18 +1925,27 @@ { PACB pACB = (PACB) scsi_device->host->hostdata; PDCB pDCB = dc390_findDCB (pACB, scsi_device->id, scsi_device->lun);; + pACB->scan_devices = 0; if (pDCB != NULL) dc390_remove_dev(pACB, pDCB); else printk(KERN_ERR"%s() called for non-existing device!\n", __FUNCTION__); } - + +static int dc390_slave_configure(struct scsi_device *scsi_device) +{ + PACB pACB = (PACB) scsi_device->host->hostdata; + pACB->scan_devices = 0; + return 0; +} + static Scsi_Host_Template driver_template = { .module = THIS_MODULE, .proc_name = "tmscsim", .proc_info = DC390_proc_info, .name = DC390_BANNER " V" DC390_VERSION, .slave_alloc = dc390_slave_alloc, + .slave_configure = dc390_slave_configure, .slave_destroy = dc390_slave_destroy, .queuecommand = DC390_queue_command, .eh_abort_handler = DC390_abort, diff -ur --exclude=CVS a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h --- a/drivers/scsi/tmscsim.h Thu May 27 22:37:18 2004 +++ b/drivers/scsi/tmscsim.h Thu May 27 22:52:39 2004 @@ -22,8 +22,6 @@ #define SEL_TIMEOUT 153 /* 250 ms selection timeout (@ 40 MHz) */ -#define END_SCAN 2 - #define pci_dma_lo32(a) (a & 0xffffffff) typedef u8 UCHAR; /* 8 bits */ ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-27 21:20 ` Guennadi Liakhovetski @ 2004-05-28 13:00 ` Christoph Hellwig 2004-05-29 15:36 ` James Bottomley 1 sibling, 0 replies; 35+ messages in thread From: Christoph Hellwig @ 2004-05-28 13:00 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list On Thu, May 27, 2004 at 11:20:50PM +0200, Guennadi Liakhovetski wrote: > On Wed, 26 May 2004, Christoph Hellwig wrote: > > > Looks like a way to go. I'm not sure what scan_device is supposed to > > do as I already wrote, but from looking at when it's set/cleared I'm > > pretty sure it tries to indicate whethere we're currently scanning for > > devices. And useing slave_alloc/slave_configure is a much safer way to > > find that out then guessing from the commands sent. > > Attached is a patch, that moves scan_device to > slave_alloc/_configure/_destroy, as you suggested (to be precise, as I > understood your suggestion:-)). Please consider. The patch also contains > some more clean-up. Don't know, if you, James, would prefer it as a > separate patch, or left alone altogether. Those minor details are > disturbing the eye, but are, perhaps, not important enough to bother you > with a separate patch:-) But if you want it separate - no problem, will do > that. > > As usual, tested on my AM53C974. Looks good to me. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: new interfaces 2004-05-27 21:20 ` Guennadi Liakhovetski 2004-05-28 13:00 ` Christoph Hellwig @ 2004-05-29 15:36 ` James Bottomley 1 sibling, 0 replies; 35+ messages in thread From: James Bottomley @ 2004-05-29 15:36 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Christoph Hellwig, Kurt Garloff, Linux SCSI list On Thu, 2004-05-27 at 16:20, Guennadi Liakhovetski wrote: > Attached is a patch, that moves scan_device to > slave_alloc/_configure/_destroy, as you suggested (to be precise, as I > understood your suggestion:-)). Please consider. The patch also contains > some more clean-up. Don't know, if you, James, would prefer it as a > separate patch, or left alone altogether. Those minor details are > disturbing the eye, but are, perhaps, not important enough to bother you > with a separate patch:-) But if you want it separate - no problem, will do > that. Well, I'll leave that up to you. It is nice clearly to separate different updates in different patches ... but I have drivers that go into the tree containing a bunch of different changes with a single comment like "update to version xxx", so I don't insist on this. James ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: Store pDCB in device->hostdata 2004-05-23 10:37 ` Christoph Hellwig 2004-05-23 21:19 ` Guennadi Liakhovetski @ 2004-05-30 21:01 ` Guennadi Liakhovetski 2004-05-31 9:22 ` Christoph Hellwig 2004-05-31 21:13 ` [PATCH] tmscsim: init / exit cleanup Guennadi Liakhovetski 2 siblings, 1 reply; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-30 21:01 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list [-- Attachment #1: Type: TEXT/PLAIN, Size: 452 bytes --] On Sun, 23 May 2004, Christoph Hellwig wrote: > The other thing is that most calls to dc390_findDCB could probably be > replaced with stroing the pDCB in scsi_device->hostdata on slave_alloc, > but that could aswell be done in an incremental patch. Done in the attached patch. Tested as usual and a bit harder. BTW, just managed to re-organise my test-system to allow modular tmscsim. Tested - works too. Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 2615 bytes --] diff --exclude=CVS -ur a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c --- a/drivers/scsi/tmscsim.c Sat May 29 22:33:30 2004 +++ b/drivers/scsi/tmscsim.c Fri May 28 23:17:17 2004 @@ -1042,7 +1042,7 @@ static int DC390_queue_command (Scsi_Cmnd *cmd, void (* done)(Scsi_Cmnd *)) { - PDCB pDCB; + PDCB pDCB = (PDCB) cmd->device->hostdata; PSRB pSRB; PACB pACB = (PACB) cmd->device->host->hostdata; @@ -1059,8 +1059,6 @@ goto fail; } - pDCB = dc390_findDCB (pACB, cmd->device->id, cmd->device->lun); - /* Should it be: BUG_ON(!pDCB); ? */ if (!pDCB) @@ -1286,7 +1284,7 @@ static int DC390_abort (Scsi_Cmnd *cmd) { - PDCB pDCB; + PDCB pDCB = (PDCB) cmd->device->hostdata; PSRB pSRB, psrb; UINT count, i; int status; @@ -1296,7 +1294,6 @@ printk ("DC390: Abort command (pid %li, Device %02i-%02i)\n", cmd->pid, cmd->device->id, cmd->device->lun); - pDCB = dc390_findDCB (pACB, cmd->device->id, cmd->device->lun); if( !pDCB ) goto NOT_RUN; /* Added 98/07/02 KG */ @@ -1909,6 +1906,7 @@ PACB pACB = (PACB) scsi_device->host->hostdata; dc390_initDCB(pACB, &pDCB, scsi_device->id, scsi_device->lun); if (pDCB != NULL) { + scsi_device->hostdata = pDCB; pACB->scan_devices = 1; return 0; } @@ -1924,7 +1922,7 @@ static void dc390_slave_destroy(struct scsi_device *scsi_device) { PACB pACB = (PACB) scsi_device->host->hostdata; - PDCB pDCB = dc390_findDCB (pACB, scsi_device->id, scsi_device->lun);; + PDCB pDCB = (PDCB) scsi_device->hostdata; pACB->scan_devices = 0; if (pDCB != NULL) dc390_remove_dev(pACB, pDCB); @@ -1978,7 +1976,7 @@ if (!scsi_host) goto nomem; - pACB = (PACB)scsi_host->hostdata; + pACB = (PACB) scsi_host->hostdata; if (dc390_init(scsi_host, io_port, irq, dev, dc390_adapterCnt)) { ret = -EBUSY; @@ -2180,7 +2178,7 @@ static int dc390_shutdown (struct Scsi_Host *host) { UCHAR bval; - PACB pACB = (PACB)(host->hostdata); + PACB pACB = (PACB) host->hostdata; /* pACB->soft_reset(host); */ @@ -2200,7 +2198,7 @@ static void dc390_freeDCBs (struct Scsi_Host *host) { PDCB pDCB, nDCB; - PACB pACB = (PACB)(host->hostdata); + PACB pACB = (PACB) host->hostdata; pDCB = pACB->pLinkDCB; if (!pDCB) return; @@ -2219,7 +2217,7 @@ static int DC390_release (struct Scsi_Host *host) { DC390_IFLAGS; - PACB pACB = (PACB)(host->hostdata); + PACB pACB = (PACB) host->hostdata; DC390_LOCK_IO(host); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: Store pDCB in device->hostdata 2004-05-30 21:01 ` [PATCH] tmscsim: Store pDCB in device->hostdata Guennadi Liakhovetski @ 2004-05-31 9:22 ` Christoph Hellwig 0 siblings, 0 replies; 35+ messages in thread From: Christoph Hellwig @ 2004-05-31 9:22 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list On Sun, May 30, 2004 at 11:01:31PM +0200, Guennadi Liakhovetski wrote: > On Sun, 23 May 2004, Christoph Hellwig wrote: > > > The other thing is that most calls to dc390_findDCB could probably be > > replaced with stroing the pDCB in scsi_device->hostdata on slave_alloc, > > but that could aswell be done in an incremental patch. > > Done in the attached patch. Tested as usual and a bit harder. > > BTW, just managed to re-organise my test-system to allow modular tmscsim. > Tested - works too. Patch looks good to me. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: init / exit cleanup 2004-05-23 10:37 ` Christoph Hellwig 2004-05-23 21:19 ` Guennadi Liakhovetski 2004-05-30 21:01 ` [PATCH] tmscsim: Store pDCB in device->hostdata Guennadi Liakhovetski @ 2004-05-31 21:13 ` Guennadi Liakhovetski 2004-06-03 13:38 ` Christoph Hellwig 2004-06-08 6:37 ` Kurt Garloff 2 siblings, 2 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-31 21:13 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list [-- Attachment #1: Type: TEXT/PLAIN, Size: 707 bytes --] On Sun, 23 May 2004, Christoph Hellwig wrote: > these leaks. Maybe also merge dc390_init and dc390_init_one? > > Similarly I think DC390_release should be merged into dc390_remove_one. Attached. Also fixed some __init / __devinit and __exit / __devexit attributes. Although, would be good to have something like #ifdef CONFIG_HOTPLUG_PCI #define __pcidevinit #define __pcidevinitdata #define __pcidevexit #define __pcidevexitdata #else #define __pcidevinit __init #define __pcidevinitdata __initdata #define __pcidevexit __exit #define __pcidevexitdata __exitdata #endif I don't think there are any pcmcia / usb / ieee1394 / ... tmscsim adapters out there? Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 12313 bytes --] --- a/drivers/scsi/tmscsim.c Mon May 31 21:41:18 2004 +++ b/drivers/scsi/tmscsim.c Mon May 31 22:28:50 2004 @@ -306,10 +306,6 @@ #include "tmscsim.h" -#ifndef __init -# define __init -#endif - static UCHAR dc390_StartSCSI( PACB pACB, PDCB pDCB, PSRB pSRB ); static void dc390_DataOut_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); static void dc390_DataIn_0( PACB pACB, PSRB pSRB, PUCHAR psstatus); @@ -343,8 +339,6 @@ static void dc390_initDCB( PACB pACB, PDCB *ppDCB, UCHAR id, UCHAR lun); static void dc390_updateDCB (PACB pACB, PDCB pDCB); -static int DC390_release(struct Scsi_Host *host); -static int dc390_shutdown (struct Scsi_Host *host); static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start, off_t offset, int length, int inout); @@ -433,7 +427,7 @@ **********************************************************************/ -static void __init dc390_EnDisableCE( UCHAR mode, PDEVDECL, PUCHAR regval ) +static void __devinit dc390_EnDisableCE( UCHAR mode, PDEVDECL, PUCHAR regval ) { UCHAR bval; @@ -450,7 +444,7 @@ /* Override EEprom values with explicitly set values */ -static void __init dc390_EEprom_Override (UCHAR index) +static void __devinit dc390_EEprom_Override (UCHAR index) { PUCHAR ptr; UCHAR id; @@ -477,7 +471,7 @@ } /* Handle "-1" case */ -static void __init dc390_check_for_safe_settings (void) +static void __devinit dc390_check_for_safe_settings (void) { if (tmscsim[0] == -1 || tmscsim[0] > 15) /* modules-2.0.0 passes -1 as string */ { @@ -500,7 +494,7 @@ , 3 /* 16 Tags per LUN */, 1 /* s delay after Reset */ }; /* Copy defaults over set values where missing */ -static void __init dc390_fill_with_defaults (void) +static void __devinit dc390_fill_with_defaults (void) { int i; PARSEDEBUG(printk(KERN_INFO "DC390: setup %08x %08x %08x %08x %08x %08x\n", tmscsim[0],\ @@ -517,6 +511,7 @@ if (tmscsim[5] > 180) tmscsim[5] = 180; } +#ifndef MODULE /* Override defaults on cmdline: * tmscsim: AdaptID, MaxSpeed (Index), DevMode (Bitmapped), AdaptMode (Bitmapped) */ @@ -536,11 +531,11 @@ /* dc390_checkparams (); */ return 1; } -#ifndef MODULE + __setup("tmscsim=", dc390_setup); #endif -static void __init dc390_EEpromOutDI( PDEVDECL, PUCHAR regval, UCHAR Carry ) +static void __devinit dc390_EEpromOutDI( PDEVDECL, PUCHAR regval, UCHAR Carry ) { UCHAR bval; @@ -561,7 +556,7 @@ } -static UCHAR __init dc390_EEpromInDO( PDEVDECL ) +static UCHAR __devinit dc390_EEpromInDO( PDEVDECL ) { UCHAR bval; @@ -577,7 +572,7 @@ } -static USHORT __init dc390_EEpromGetData1( PDEVDECL ) +static USHORT __devinit dc390_EEpromGetData1( PDEVDECL ) { UCHAR i; UCHAR carryFlag; @@ -594,7 +589,7 @@ } -static void __init dc390_Prepare( PDEVDECL, PUCHAR regval, UCHAR EEpromCmd ) +static void __devinit dc390_Prepare( PDEVDECL, PUCHAR regval, UCHAR EEpromCmd ) { UCHAR i,j; UCHAR carryFlag; @@ -610,7 +605,7 @@ } -static void __init dc390_ReadEEprom( PDEVDECL, PUSHORT ptr) +static void __devinit dc390_ReadEEprom( PDEVDECL, PUSHORT ptr) { UCHAR regval,cmd; UCHAR i; @@ -626,13 +621,13 @@ } -static void __init dc390_interpret_delay (UCHAR index) +static void __devinit dc390_interpret_delay (UCHAR index) { char interpd [] = {1,3,5,10,16,30,60,120}; dc390_eepromBuf[index][EE_DELAY] = interpd [dc390_eepromBuf[index][EE_DELAY]]; } -static UCHAR __init dc390_CheckEEpromCheckSum( PDEVDECL, UCHAR index ) +static UCHAR __devinit dc390_CheckEEpromCheckSum( PDEVDECL, UCHAR index ) { UCHAR i; char EEbuf[128]; @@ -1684,7 +1679,7 @@ * io_port, Irq, index: Resources and adapter index ***********************************************************************/ -static void __init dc390_initACB (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) +static void __devinit dc390_initACB (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) { PACB pACB; UCHAR i; @@ -1756,7 +1751,7 @@ * Outputs: 0 on success, -1 on error ***********************************************************************/ -static int __init dc390_initAdapter (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) +static int __devinit dc390_initAdapter (PSH psh, ULONG io_port, UCHAR Irq, UCHAR index) { PACB pACB, pACB2; UCHAR dstate; @@ -1827,65 +1822,7 @@ } -/*********************************************************************** - * Function : static int DC390_init (struct Scsi_Host *host, ...) - * - * Purpose : initialize the internal structures for a given SCSI host - * - * Inputs : host - pointer to this host adapter's structure - * io_port - IO ports mapped to this adapter - * irq - IRQ assigned to this adpater - * struct pci_dev - PCI access handle - * index - Adapter index - * - * Outputs: 0 on success, -1 on error - * - * Note: written in capitals, because the locking is only done here, - * not in DC390_detect, called from outside - ***********************************************************************/ -static int __init dc390_init (PSH psh, unsigned long io_port, u8 irq, struct pci_dev *pdev, UCHAR index) -{ - PACB pACB; - - if (dc390_CheckEEpromCheckSum (PDEV, index)) - { - int speed; - dc390_adapname = "AM53C974"; - printk (KERN_INFO "DC390_init: No EEPROM found! Trying default settings ...\n"); - dc390_check_for_safe_settings (); - dc390_fill_with_defaults (); - dc390_EEprom_Override (index); - speed = dc390_clock_speed[tmscsim[1]]; - printk (KERN_INFO "DC390: Used defaults: AdaptID=%i, SpeedIdx=%i (%i.%i MHz)," - " DevMode=0x%02x, AdaptMode=0x%02x, TaggedCmnds=%i (%i), DelayReset=%is\n", - tmscsim[0], tmscsim[1], speed/10, speed%10, - (UCHAR)tmscsim[2], (UCHAR)tmscsim[3], tmscsim[4], 2 << (tmscsim[4]), tmscsim[5]); - } - else - { - dc390_check_for_safe_settings (); - dc390_EEprom_Override (index); - } - pACB = (PACB) psh->hostdata; - - DEBUG0(printk(KERN_INFO "DC390: pSH = %8x, Index %02i\n", (UINT) psh, index)); - - dc390_initACB( psh, io_port, irq, index ); - - PDEVSET; - - if( !dc390_initAdapter( psh, io_port, irq, index ) ) - { - return (0); - } - else - { - scsi_unregister( psh ); - return( -1 ); - } -} - -static void __init dc390_set_pci_cfg (PDEVDECL) +static void __devinit dc390_set_pci_cfg (PDEVDECL) { USHORT cmd; PCI_READ_CONFIG_WORD (PDEV, PCI_COMMAND, &cmd); @@ -1978,7 +1915,31 @@ pACB = (PACB) scsi_host->hostdata; - if (dc390_init(scsi_host, io_port, irq, dev, dc390_adapterCnt)) { + if (dc390_CheckEEpromCheckSum (dev, dc390_adapterCnt)) { + int speed; + dc390_adapname = "AM53C974"; + printk(KERN_INFO "DC390_init: No EEPROM found! Trying default settings ...\n"); + dc390_check_for_safe_settings(); + dc390_fill_with_defaults(); + dc390_EEprom_Override(dc390_adapterCnt); + speed = dc390_clock_speed[tmscsim[1]]; + printk(KERN_INFO "DC390: Used defaults: AdaptID=%i, SpeedIdx=%i (%i.%i MHz)," + " DevMode=0x%02x, AdaptMode=0x%02x, TaggedCmnds=%i (%i), DelayReset=%is\n", + tmscsim[0], tmscsim[1], speed/10, speed%10, + (UCHAR)tmscsim[2], (UCHAR)tmscsim[3], tmscsim[4], 2 << (tmscsim[4]), tmscsim[5]); + } else { + dc390_check_for_safe_settings(); + dc390_EEprom_Override(dc390_adapterCnt); + } + + DEBUG0(printk(KERN_INFO "DC390: pSH = %8x, Index %02i\n", (UINT) scsi_host, dc390_adapterCnt)); + + dc390_initACB(scsi_host, io_port, irq, dc390_adapterCnt); + + pACB->pdev = dev; + + if (dc390_initAdapter(scsi_host, io_port, irq, dc390_adapterCnt)) { + scsi_unregister(scsi_host); ret = -EBUSY; goto busy; } @@ -2005,6 +1966,52 @@ return ret; } +static void __devexit dc390_freeDCBs (struct Scsi_Host *host) +{ + PDCB pDCB, nDCB; + PACB pACB = (PACB) host->hostdata; + + pDCB = pACB->pLinkDCB; + if (!pDCB) return; + do + { + nDCB = pDCB->pNextDCB; + DCBDEBUG(printk (KERN_INFO "DC390: Free DCB (ID %i, LUN %i): %p\n",\ + pDCB->TargetID, pDCB->TargetLUN, pDCB)); + //kfree (pDCB); + dc390_remove_dev (pACB, pDCB); + pDCB = nDCB; + } while (pDCB && pACB->pLinkDCB); +} + +/*********************************************************************** + * Function : static int dc390_shutdown (struct Scsi_Host *host) + * + * Purpose : does a clean (we hope) shutdown of the SCSI chip. + * Use prior to dumping core, unloading the driver, etc. + * + * Returns : 0 on success + ***********************************************************************/ +static int __devexit dc390_shutdown (struct Scsi_Host *host) +{ + UCHAR bval; + PACB pACB = (PACB) host->hostdata; + +/* pACB->soft_reset(host); */ + + printk(KERN_INFO "DC390: shutdown\n"); + + pACB->ACBFlag = RESET_DEV; + bval = DC390_read8 (CtrlReg1); + bval |= DIS_INT_ON_SCSI_RST; + DC390_write8 (CtrlReg1, bval); /* disable interrupt */ + if (pACB->Gmode2 & RST_SCSI_BUS) + dc390_ResetSCSIBus (pACB); + + if (timer_pending (&pACB->Waiting_Timer)) del_timer (&pACB->Waiting_Timer); + return( 0 ); +} + /** * dc390_remove_one - Called to remove a single instance of the adapter. * @@ -2013,9 +2020,25 @@ static void __devexit dc390_remove_one(struct pci_dev *dev) { struct Scsi_Host *scsi_host = pci_get_drvdata(dev); + DC390_IFLAGS; + PACB pACB = (PACB) scsi_host->hostdata; scsi_remove_host(scsi_host); - DC390_release(scsi_host); + + DC390_LOCK_IO(scsi_host); + + /* TO DO: We should check for outstanding commands first. */ + dc390_shutdown(scsi_host); + + if (scsi_host->irq != SCSI_IRQ_NONE) { + DEBUG0(printk(KERN_INFO "DC390: Free IRQ %i\n", scsi_host->irq)); + free_irq(scsi_host->irq, pACB); + } + + release_region(scsi_host->io_port, scsi_host->n_io_port); + dc390_freeDCBs(scsi_host); + DC390_UNLOCK_IO(scsi_host); + pci_disable_device(dev); scsi_host_put(scsi_host); pci_set_drvdata(dev, NULL); @@ -2166,75 +2189,6 @@ #undef YESNO #undef SPRINTF - -/*********************************************************************** - * Function : static int dc390_shutdown (struct Scsi_Host *host) - * - * Purpose : does a clean (we hope) shutdown of the SCSI chip. - * Use prior to dumping core, unloading the driver, etc. - * - * Returns : 0 on success - ***********************************************************************/ -static int dc390_shutdown (struct Scsi_Host *host) -{ - UCHAR bval; - PACB pACB = (PACB) host->hostdata; - -/* pACB->soft_reset(host); */ - - printk(KERN_INFO "DC390: shutdown\n"); - - pACB->ACBFlag = RESET_DEV; - bval = DC390_read8 (CtrlReg1); - bval |= DIS_INT_ON_SCSI_RST; - DC390_write8 (CtrlReg1, bval); /* disable interrupt */ - if (pACB->Gmode2 & RST_SCSI_BUS) - dc390_ResetSCSIBus (pACB); - - if (timer_pending (&pACB->Waiting_Timer)) del_timer (&pACB->Waiting_Timer); - return( 0 ); -} - -static void dc390_freeDCBs (struct Scsi_Host *host) -{ - PDCB pDCB, nDCB; - PACB pACB = (PACB) host->hostdata; - - pDCB = pACB->pLinkDCB; - if (!pDCB) return; - do - { - nDCB = pDCB->pNextDCB; - DCBDEBUG(printk (KERN_INFO "DC390: Free DCB (ID %i, LUN %i): %p\n",\ - pDCB->TargetID, pDCB->TargetLUN, pDCB)); - //kfree (pDCB); - dc390_remove_dev (pACB, pDCB); - pDCB = nDCB; - } while (pDCB && pACB->pLinkDCB); - -} - -static int DC390_release (struct Scsi_Host *host) -{ - DC390_IFLAGS; - PACB pACB = (PACB) host->hostdata; - - DC390_LOCK_IO(host); - - /* TO DO: We should check for outstanding commands first. */ - dc390_shutdown (host); - - if (host->irq != SCSI_IRQ_NONE) - { - DEBUG0(printk(KERN_INFO "DC390: Free IRQ %i\n",host->irq)); - free_irq (host->irq, pACB); - } - - release_region(host->io_port,host->n_io_port); - dc390_freeDCBs (host); - DC390_UNLOCK_IO(host); - return( 1 ); -} static struct pci_driver dc390_driver = { .name = "tmscsim", ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: init / exit cleanup 2004-05-31 21:13 ` [PATCH] tmscsim: init / exit cleanup Guennadi Liakhovetski @ 2004-06-03 13:38 ` Christoph Hellwig 2004-06-03 20:23 ` Guennadi Liakhovetski 2004-06-08 6:37 ` Kurt Garloff 1 sibling, 1 reply; 35+ messages in thread From: Christoph Hellwig @ 2004-06-03 13:38 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Christoph Hellwig, James Bottomley, Kurt Garloff, Linux SCSI list On Mon, May 31, 2004 at 11:13:51PM +0200, Guennadi Liakhovetski wrote: > On Sun, 23 May 2004, Christoph Hellwig wrote: > > > these leaks. Maybe also merge dc390_init and dc390_init_one? > > > > Similarly I think DC390_release should be merged into dc390_remove_one. > > Attached. Also fixed some __init / __devinit and __exit / __devexit > attributes. Although, would be good to have something like > > #ifdef CONFIG_HOTPLUG_PCI > #define __pcidevinit > #define __pcidevinitdata > #define __pcidevexit > #define __pcidevexitdata > #else > #define __pcidevinit __init > #define __pcidevinitdata __initdata > #define __pcidevexit __exit > #define __pcidevexitdata __exitdata > #endif > > I don't think there are any pcmcia / usb / ieee1394 / ... tmscsim adapters > out there? This has been though out a bit but not considered worthwile. And to actually work it would have to be for all the kernel because you could get problems over the module boundary otherwise. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: init / exit cleanup 2004-06-03 13:38 ` Christoph Hellwig @ 2004-06-03 20:23 ` Guennadi Liakhovetski 2004-06-06 17:09 ` Christoph Hellwig 0 siblings, 1 reply; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-06-03 20:23 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list On Thu, 3 Jun 2004, Christoph Hellwig wrote: > On Mon, May 31, 2004 at 11:13:51PM +0200, Guennadi Liakhovetski wrote: > > > > #ifdef CONFIG_HOTPLUG_PCI > > #define __pcidevinit > > #define __pcidevinitdata > > #define __pcidevexit > > #define __pcidevexitdata > > #else > > #define __pcidevinit __init > > #define __pcidevinitdata __initdata > > #define __pcidevexit __exit > > #define __pcidevexitdata __exitdata > > #endif > > This has been though out a bit but not considered worthwile. And to actually > work it would have to be for all the kernel because you could get problems > over the module boundary otherwise. Sorry, do you have a reference or remember at least apporoximatly when and where the discussion took place? I don't quite understand your arguments. We do have __dev{init,exit,...} macros, do they have "problems over the module boundary"? If yes - they want to be solved, right? If not - why would equally defined __pcidev* have problems? You mean using __devinit in a module has probles? The advantages of the above are obvious: there are many more HOTPLUG-enabled systems, than PCI_HOTPLUG-enabled ones. And a reasonable number of drivers, that serve devices only over PCI. How much would it add to "Freeing init memory" in an "average" configuration? Thanks Guennadi --- Guennadi Liakhovetski ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: init / exit cleanup 2004-06-03 20:23 ` Guennadi Liakhovetski @ 2004-06-06 17:09 ` Christoph Hellwig 0 siblings, 0 replies; 35+ messages in thread From: Christoph Hellwig @ 2004-06-06 17:09 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: James Bottomley, Kurt Garloff, Linux SCSI list On Thu, Jun 03, 2004 at 10:23:45PM +0200, Guennadi Liakhovetski wrote: > Sorry, do you have a reference or remember at least apporoximatly when and > where the discussion took place? Sorry, don't remember anymore. Try searching the lkml archives. > I don't quite understand your arguments. > We do have __dev{init,exit,...} macros, do they have "problems over the > module boundary"? If yes - they want to be solved, right? currently you can have pointers to __devinitdata functions in structures registered with other modules. Having more different lifetime rules will complicate that enormously and need a audit of the whole kernel first. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: init / exit cleanup 2004-05-31 21:13 ` [PATCH] tmscsim: init / exit cleanup Guennadi Liakhovetski 2004-06-03 13:38 ` Christoph Hellwig @ 2004-06-08 6:37 ` Kurt Garloff 1 sibling, 0 replies; 35+ messages in thread From: Kurt Garloff @ 2004-06-08 6:37 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Christoph Hellwig, James Bottomley, Linux SCSI list [-- Attachment #1: Type: text/plain, Size: 356 bytes --] Hi, On Mon, May 31, 2004 at 11:13:51PM +0200, Guennadi Liakhovetski wrote: > I don't think there are any pcmcia / usb / ieee1394 / ... tmscsim adapters > out there? Never heard of any. Regards, -- Kurt Garloff <garloff@suse.de> Cologne, DE SUSE LINUX AG / Novell, Nuernberg, DE Director SUSE Labs [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-05-22 15:43 ` James Bottomley 2004-05-22 18:10 ` Kurt Garloff 2004-05-22 19:34 ` [PATCH] tmscsim: new interfaces Guennadi Liakhovetski @ 2004-05-22 19:38 ` Guennadi Liakhovetski 2 siblings, 0 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-05-22 19:38 UTC (permalink / raw) To: James Bottomley; +Cc: Kurt Garloff, Linux SCSI list, Christoph Hellwig On 22 May 2004, James Bottomley wrote: > On Sat, 2004-05-22 at 03:09, Kurt Garloff wrote: > > We could just define our own format, make a copy and then use that one. > > Sure, that would be fine. Whatever the registers take. Thanks for clarifications and ideas, Kurt, James. As usual - any help is highly appreciated. Regards Guennadi --- Guennadi Liakhovetski ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-05-21 15:15 ` [PATCH] tmscsim: 64-bit cleanup James Bottomley 2004-05-21 22:53 ` Guennadi Liakhovetski 2004-05-22 8:09 ` Kurt Garloff @ 2004-06-04 22:17 ` Guennadi Liakhovetski 2004-06-05 8:12 ` Guennadi Liakhovetski 2004-06-05 14:02 ` James Bottomley 2 siblings, 2 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-06-04 22:17 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: TEXT/PLAIN, Size: 2414 bytes --] On 21 May 2004, James Bottomley wrote: > On Wed, 2004-05-19 at 14:50, Guennadi Liakhovetski wrote: > > Ok, here comes the first one. I chose this one because it fixes an actual > > bug in the driver. This bug was (partially) introduced by myself when > > porting to 2.6. Partly the reason was that I disliked using > > function-like macros as lvalues: > > > > sg_dma_address(x) = ... > > sg_dma_len(x) = ... > > > > [OT] wouldn't it be better to introduce some macros like > > set_sg_dma_{address|len}(x, y)? > > Actually, no. struct scatterlist is for the OS platform to use to > characterise DMA; it's actually a private structure entirely within the > gift of the architecture to define. It usually contains bits of > extraneous data that the driver never sees. The driver should convert > struct scatterlist into its own version of the scatterlist that needs > feeding to the hardware rather than try and use struct scatterlist. Well, I don't quite understand. What about this code in block/ll_rw_blk.c: sg[nsegs].page = bvec->bv_page; sg[nsegs].length = nbytes; sg[nsegs].offset = bvec->bv_offset; ? So, looks like all architectures should implement these fields in struct scatterlist, and use them in map_sg. So, isn't it safe to use them from drivers in a similar way (see patch attached). > You'll find that lvalue use of sg_dma_len() fails on some platforms that > actually compute it from page/offset values in the scatterlist. Actually, I found only 1 platform, where sg_dma_address() cannot be used as an lvalue: include/asm-sh/pci.h:#define sg_dma_address(sg) (virt_to_bus((sg)->dma_address)) > Could you (eventually) update tmscsim not to use it? Don't know if this is correct, but the most consistent way to do it, I was able to find, was to reproduce the code above for an sg with 1 element and call pci_map_sg() instead of pci_map_page, thus avoiding assignments like sg_dma_len(sg) = ...; sg_dma_address(sg) = ...; There's still one use of sg_dma_len() as an lvalue left in scsiiom.c on the RESTORE_PTR path. But (1) at the moment it seems to be safe on all platforms, (2) I am very suspicious about that code any way and would appreciate any advise as to if that code is correct, and, if not, what should be done there. Emn, ok, in case it is already compalsory, here goes: Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 3294 bytes --] --- a/drivers/scsi/tmscsim.c Fri Jun 04 21:41:18 2004 +++ b/drivers/scsi/tmscsim.c Fri Jun 04 23:28:50 2004 @@ -919,18 +919,8 @@ Scsi_Cmnd *pcmd = pSRB->pcmd; struct pci_dev *pdev = pSRB->pSRBDCB->pDCBACB->pdev; dc390_cmd_scp_t* cmdp = ((dc390_cmd_scp_t*)(&pcmd->SCp)); - /* Map sense buffer */ - if (pSRB->SRBFlag & AUTO_REQSENSE) { - sg_dma_address(&pSRB->Segmentx) = cmdp->saved_dma_handle = - pci_map_page(pdev, virt_to_page(pcmd->sense_buffer), - (unsigned long)pcmd->sense_buffer & ~PAGE_MASK, sizeof(pcmd->sense_buffer), - DMA_FROM_DEVICE); - sg_dma_len(&pSRB->Segmentx) = sizeof(pcmd->sense_buffer); - pSRB->SGcount = 1; - pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; - DEBUG1(printk("%s(): Mapped sense buffer %p at %x\n", __FUNCTION__, pcmd->sense_buffer, cmdp->saved_dma_handle)); - /* Make SG list */ - } else if (pcmd->use_sg) { + /* Map SG list */ + if (pcmd->use_sg) { pSRB->pSegmentList = (PSGL) pcmd->request_buffer; pSRB->SGcount = pci_map_sg(pdev, pSRB->pSegmentList, pcmd->use_sg, @@ -938,18 +928,37 @@ /* TODO: error handling */ if (!pSRB->SGcount) error = 1; - DEBUG1(printk("%s(): Mapped SG %p with %d (%d) elements\n", __FUNCTION__, pcmd->request_buffer, pSRB->SGcount, pcmd->use_sg)); - /* Map single segment */ - } else if (pcmd->request_buffer && pcmd->request_bufflen) { - sg_dma_address(&pSRB->Segmentx) = cmdp->saved_dma_handle = - pci_map_page(pdev, virt_to_page(pcmd->request_buffer), - (unsigned long)pcmd->request_buffer & ~PAGE_MASK, - pcmd->request_bufflen, scsi_to_pci_dma_dir(pcmd->sc_data_direction)); + DEBUG1(printk("%s(): Mapped SG %p with %d (%d) elements\n",\ + __FUNCTION__, pcmd->request_buffer, pSRB->SGcount, pcmd->use_sg)); + } else if ((pSRB->SRBFlag & AUTO_REQSENSE) || (pcmd->request_buffer && pcmd->request_bufflen)) { + void *addr; + unsigned long length; + int dir; + + if (pSRB->SRBFlag & AUTO_REQSENSE) { + /* Map sense buffer */ + addr = pcmd->sense_buffer; + length = sizeof(pcmd->sense_buffer); + dir = DMA_FROM_DEVICE; + DEBUG1(printk("%s(): Mapped sense buffer %p", __FUNCTION__, pcmd->sense_buffer)); + } else { + /* Map single segment */ + addr = pcmd->request_buffer; + length = pcmd->request_bufflen; + dir = scsi_to_pci_dma_dir(pcmd->sc_data_direction); + DEBUG1(printk("%s(): Mapped request buffer %p", __FUNCTION__, pcmd->request_buffer)); + } + memset(&pSRB->Segmentx, 0, sizeof(struct scatterlist)); + pSRB->Segmentx.page = virt_to_page(addr); + pSRB->Segmentx.length = length; + pSRB->Segmentx.offset = (unsigned long)addr & ~PAGE_MASK; + pSRB->pSegmentList = &pSRB->Segmentx; + pSRB->SGcount = pci_map_sg(pdev, &pSRB->Segmentx, 1, dir); + cmdp->saved_dma_handle = sg_dma_address(&pSRB->Segmentx); + DEBUG1(printk(" at %x\n", cmdp->saved_dma_handle)); /* TODO: error handling */ - sg_dma_len(&pSRB->Segmentx) = pcmd->request_bufflen; - pSRB->SGcount = 1; - pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; - DEBUG1(printk("%s(): Mapped request buffer %p at %x\n", __FUNCTION__, pcmd->request_buffer, cmdp->saved_dma_handle)); + if (pSRB->SGcount != 1) + error = 1; /* No mapping !? */ } else pSRB->SGcount = 0; ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-06-04 22:17 ` Guennadi Liakhovetski @ 2004-06-05 8:12 ` Guennadi Liakhovetski 2004-06-05 14:02 ` James Bottomley 1 sibling, 0 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-06-05 8:12 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: TEXT/PLAIN, Size: 476 bytes --] On Sat, 5 Jun 2004, Guennadi Liakhovetski wrote: > scatterlist, and use them in map_sg. So, isn't it safe to use them from > drivers in a similar way (see patch attached). Ooops, forgot to modify the unmapping path. New version attached (I'm looking forward to getting hold of the new version of SubmittingPatches, meanwhile, I guess, I have to sign-off for it again) Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 4753 bytes --] --- a/drivers/scsi/tmscsim.c Fri Jun 04 21:41:18 2004 +++ b/drivers/scsi/tmscsim.c Fri Jun 04 23:28:50 2004 @@ -919,18 +919,8 @@ Scsi_Cmnd *pcmd = pSRB->pcmd; struct pci_dev *pdev = pSRB->pSRBDCB->pDCBACB->pdev; dc390_cmd_scp_t* cmdp = ((dc390_cmd_scp_t*)(&pcmd->SCp)); - /* Map sense buffer */ - if (pSRB->SRBFlag & AUTO_REQSENSE) { - sg_dma_address(&pSRB->Segmentx) = cmdp->saved_dma_handle = - pci_map_page(pdev, virt_to_page(pcmd->sense_buffer), - (unsigned long)pcmd->sense_buffer & ~PAGE_MASK, sizeof(pcmd->sense_buffer), - DMA_FROM_DEVICE); - sg_dma_len(&pSRB->Segmentx) = sizeof(pcmd->sense_buffer); - pSRB->SGcount = 1; - pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; - DEBUG1(printk("%s(): Mapped sense buffer %p at %x\n", __FUNCTION__, pcmd->sense_buffer, cmdp->saved_dma_handle)); - /* Make SG list */ - } else if (pcmd->use_sg) { + /* Map SG list */ + if (pcmd->use_sg) { pSRB->pSegmentList = (PSGL) pcmd->request_buffer; pSRB->SGcount = pci_map_sg(pdev, pSRB->pSegmentList, pcmd->use_sg, @@ -938,18 +928,37 @@ /* TODO: error handling */ if (!pSRB->SGcount) error = 1; - DEBUG1(printk("%s(): Mapped SG %p with %d (%d) elements\n", __FUNCTION__, pcmd->request_buffer, pSRB->SGcount, pcmd->use_sg)); - /* Map single segment */ - } else if (pcmd->request_buffer && pcmd->request_bufflen) { - sg_dma_address(&pSRB->Segmentx) = cmdp->saved_dma_handle = - pci_map_page(pdev, virt_to_page(pcmd->request_buffer), - (unsigned long)pcmd->request_buffer & ~PAGE_MASK, - pcmd->request_bufflen, scsi_to_pci_dma_dir(pcmd->sc_data_direction)); + DEBUG1(printk("%s(): Mapped SG %p with %d (%d) elements\n",\ + __FUNCTION__, pcmd->request_buffer, pSRB->SGcount, pcmd->use_sg)); + } else if ((pSRB->SRBFlag & AUTO_REQSENSE) || (pcmd->request_buffer && pcmd->request_bufflen)) { + void *addr; + unsigned long length; + int dir; + + if (pSRB->SRBFlag & AUTO_REQSENSE) { + /* Map sense buffer */ + addr = pcmd->sense_buffer; + length = sizeof(pcmd->sense_buffer); + dir = DMA_FROM_DEVICE; + DEBUG1(printk("%s(): Mapped sense buffer %p", __FUNCTION__, pcmd->sense_buffer)); + } else { + /* Map single segment */ + addr = pcmd->request_buffer; + length = pcmd->request_bufflen; + dir = scsi_to_pci_dma_dir(pcmd->sc_data_direction); + DEBUG1(printk("%s(): Mapped request buffer %p", __FUNCTION__, pcmd->request_buffer)); + } + memset(&pSRB->Segmentx, 0, sizeof(struct scatterlist)); + pSRB->Segmentx.page = virt_to_page(addr); + pSRB->Segmentx.length = length; + pSRB->Segmentx.offset = (unsigned long)addr & ~PAGE_MASK; + pSRB->pSegmentList = &pSRB->Segmentx; + pSRB->SGcount = pci_map_sg(pdev, &pSRB->Segmentx, 1, dir); + cmdp->saved_dma_handle = sg_dma_address(&pSRB->Segmentx); + DEBUG1(printk(" at %x\n", cmdp->saved_dma_handle)); /* TODO: error handling */ - sg_dma_len(&pSRB->Segmentx) = pcmd->request_bufflen; - pSRB->SGcount = 1; - pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; - DEBUG1(printk("%s(): Mapped request buffer %p at %x\n", __FUNCTION__, pcmd->request_buffer, cmdp->saved_dma_handle)); + if (pSRB->SGcount != 1) + error = 1; /* No mapping !? */ } else pSRB->SGcount = 0; @@ -963,20 +972,21 @@ struct pci_dev *pdev = pSRB->pSRBDCB->pDCBACB->pdev; dc390_cmd_scp_t* cmdp = ((dc390_cmd_scp_t*)(&pcmd->SCp)); - if (pSRB->SRBFlag) { - pci_unmap_page(pdev, cmdp->saved_dma_handle, - sizeof(pcmd->sense_buffer), DMA_FROM_DEVICE); - DEBUG1(printk("%s(): Unmapped sense buffer at %x\n", __FUNCTION__, cmdp->saved_dma_handle)); - } else if (pcmd->use_sg) { + if (pcmd->use_sg) { pci_unmap_sg(pdev, pcmd->request_buffer, pcmd->use_sg, scsi_to_pci_dma_dir(pcmd->sc_data_direction)); DEBUG1(printk("%s(): Unmapped SG at %p with %d elements\n", __FUNCTION__, pcmd->request_buffer, pcmd->use_sg)); - } else if (pcmd->request_buffer && pcmd->request_bufflen) { - pci_unmap_page(pdev, - cmdp->saved_dma_handle, - pcmd->request_bufflen, - scsi_to_pci_dma_dir(pcmd->sc_data_direction)); - DEBUG1(printk("%s(): Unmapped request buffer at %x\n", __FUNCTION__, cmdp->saved_dma_handle)); + } else if (pSRB->SRBFlag || (pcmd->request_buffer && pcmd->request_bufflen)) { + int dir; + + if (pSRB->SRBFlag) { + dir = DMA_FROM_DEVICE; + DEBUG1(printk("%s(): Unmapped sense buffer at %x\n", __FUNCTION__, cmdp->saved_dma_handle)); + } else { + dir = scsi_to_pci_dma_dir(pcmd->sc_data_direction); + DEBUG1(printk("%s(): Unmapped request buffer at %x\n", __FUNCTION__, cmdp->saved_dma_handle)); + } + pci_unmap_sg(pdev, &pSRB->Segmentx, 1, dir); } } ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-06-04 22:17 ` Guennadi Liakhovetski 2004-06-05 8:12 ` Guennadi Liakhovetski @ 2004-06-05 14:02 ` James Bottomley 2004-06-05 20:37 ` Guennadi Liakhovetski 1 sibling, 1 reply; 35+ messages in thread From: James Bottomley @ 2004-06-05 14:02 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Linux SCSI list, Christoph Hellwig On Fri, 2004-06-04 at 17:17, Guennadi Liakhovetski wrote: > On 21 May 2004, James Bottomley wrote: > > On Wed, 2004-05-19 at 14:50, Guennadi Liakhovetski wrote: > > > Ok, here comes the first one. I chose this one because it fixes an actual > > > bug in the driver. This bug was (partially) introduced by myself when > > > porting to 2.6. Partly the reason was that I disliked using > > > function-like macros as lvalues: > > > > > > sg_dma_address(x) = ... > > > sg_dma_len(x) = ... > > > > > > [OT] wouldn't it be better to introduce some macros like > > > set_sg_dma_{address|len}(x, y)? > > > > Actually, no. struct scatterlist is for the OS platform to use to > > characterise DMA; it's actually a private structure entirely within the > > gift of the architecture to define. It usually contains bits of > > extraneous data that the driver never sees. The driver should convert > > struct scatterlist into its own version of the scatterlist that needs > > feeding to the hardware rather than try and use struct scatterlist. > > Well, I don't quite understand. What about this code in block/ll_rw_blk.c: > sg[nsegs].page = bvec->bv_page; > sg[nsegs].length = nbytes; > sg[nsegs].offset = bvec->bv_offset; > ? So, looks like all architectures should implement these fields in struct > scatterlist, and use them in map_sg. So, isn't it safe to use them from > drivers in a similar way (see patch attached). No. struct scatterlist is defined by the platform in two halves: the block side, which uses the three members you list above, and which, as you say, all architectures are required to provide and the platform side which define extra elements but use the macro accessors sg_dma_len() and sg_dma_address(). However, if you tamper with the block side of this, you'll loose idempotence (the property that dma_map_sg()->dma_unmap_sg()->dma_map_sg() works). Without this none of the retryable error paths will work. You should simply treat the scatterlist as an information providing opaque structure you examine with the provided accessors. If you try to alter it, you'll get into trouble with the platform layer, the block layer or both. James ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-06-05 14:02 ` James Bottomley @ 2004-06-05 20:37 ` Guennadi Liakhovetski 2004-06-05 21:06 ` James Bottomley 0 siblings, 1 reply; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-06-05 20:37 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig On 5 Jun 2004, James Bottomley wrote: > On Fri, 2004-06-04 at 17:17, Guennadi Liakhovetski wrote: > > Well, I don't quite understand. What about this code in block/ll_rw_blk.c: > > sg[nsegs].page = bvec->bv_page; > > sg[nsegs].length = nbytes; > > sg[nsegs].offset = bvec->bv_offset; > > ? So, looks like all architectures should implement these fields in struct > > scatterlist, and use them in map_sg. So, isn't it safe to use them from > > drivers in a similar way (see patch attached). > > No. > > struct scatterlist is defined by the platform in two halves: the block > side, which uses the three members you list above, and which, as you > say, all architectures are required to provide and the platform side > which define extra elements but use the macro accessors sg_dma_len() and > sg_dma_address(). > > However, if you tamper with the block side of this, you'll loose > idempotence (the property that > dma_map_sg()->dma_unmap_sg()->dma_map_sg() works). Without this none of > the retryable error paths will work. Thanks for replies and explanations, James! But could you, please, just explain a bit more - what and how do I break with this patch? I just use a driver-private scatterlist struct (SRB.Segmentx) to set up a single-segment sg-list, and call pci_map_sg() on it. If I do it wrong - it could be fixed. But why it is bad in principle? Advantages - no need to check if sg is used for this particular command throughout the code. But no problem. I'll redo the patch without private sg. Thanks Guennadi --- Guennadi Liakhovetski ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-06-05 20:37 ` Guennadi Liakhovetski @ 2004-06-05 21:06 ` James Bottomley 2004-06-06 15:06 ` Guennadi Liakhovetski 0 siblings, 1 reply; 35+ messages in thread From: James Bottomley @ 2004-06-05 21:06 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Linux SCSI list, Christoph Hellwig On Sat, 2004-06-05 at 15:37, Guennadi Liakhovetski wrote: > Thanks for replies and explanations, James! But could you, please, just > explain a bit more - what and how do I break with this patch? I just use a > driver-private scatterlist struct (SRB.Segmentx) to set up a > single-segment sg-list, and call pci_map_sg() on it. If I do it wrong - it > could be fixed. But why it is bad in principle? Advantages - no need to > check if sg is used for this particular command throughout the code. > > But no problem. I'll redo the patch without private sg. Actually, I haven't looked at the patch yet. I was just answering the question in the mail which I assumed was "can I alter these three fields?" to which the answer is "no". If the question is "can I read these fields directly" the answer's yes. James ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-06-05 21:06 ` James Bottomley @ 2004-06-06 15:06 ` Guennadi Liakhovetski 2004-06-14 21:37 ` Guennadi Liakhovetski 0 siblings, 1 reply; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-06-06 15:06 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig On 5 Jun 2004, James Bottomley wrote: > On Sat, 2004-06-05 at 15:37, Guennadi Liakhovetski wrote: > > Thanks for replies and explanations, James! But could you, please, just > > explain a bit more - what and how do I break with this patch? I just use a > > driver-private scatterlist struct (SRB.Segmentx) to set up a > > single-segment sg-list, and call pci_map_sg() on it. If I do it wrong - it > > could be fixed. But why it is bad in principle? Advantages - no need to > > check if sg is used for this particular command throughout the code. > > > > But no problem. I'll redo the patch without private sg. > > Actually, I haven't looked at the patch yet. I was just answering the > question in the mail which I assumed was "can I alter these three > fields?" to which the answer is "no". If the question is "can I read > these fields directly" the answer's yes. Ah, ok. I don't alter those fields in the sg, set up by the block-layer. I just set up a separate sg for cases, when the block layer doesn't do it. That helps to unify the processing. I would prefer to go that way. Although, I think, there's still a glitch there (modified if's order). I'll send a new version, if the approach is approved in principle. I also prepared a patch without extra sg's, but I am much less confident about it, although it survived some light testing. Thanks Guennadi --- Guennadi Liakhovetski ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] tmscsim: 64-bit cleanup 2004-06-06 15:06 ` Guennadi Liakhovetski @ 2004-06-14 21:37 ` Guennadi Liakhovetski 0 siblings, 0 replies; 35+ messages in thread From: Guennadi Liakhovetski @ 2004-06-14 21:37 UTC (permalink / raw) To: James Bottomley; +Cc: Linux SCSI list, Christoph Hellwig [-- Attachment #1: Type: TEXT/PLAIN, Size: 450 bytes --] On Sun, 6 Jun 2004, Guennadi Liakhovetski wrote: > Although, I think, there's still a glitch there (modified if's order). > I'll send a new version, if the approach is approved in principle. Ok, attached is version 3:-) It also fixes a definite (although, perhaps, harmless) bug in scsiiom.c. And removes a redundant assignment in tmscsim.c. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Thanks Guennadi --- Guennadi Liakhovetski [-- Attachment #2: Type: TEXT/PLAIN, Size: 6132 bytes --] diff -u a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c --- a/drivers/scsi/tmscsim.c Fri Jun 04 21:41:18 2004 +++ b/drivers/scsi/tmscsim.c Mon Jun 14 22:28:50 2004 @@ -911,6 +911,14 @@ } } +static struct scatterlist* dc390_sg_build_single(struct scatterlist *sg, void *addr, unsigned int length) +{ + memset(sg, 0, sizeof(struct scatterlist)); + sg->page = virt_to_page(addr); + sg->length = length; + sg->offset = (unsigned long)addr & ~PAGE_MASK; + return sg; +} /* Create pci mapping */ static int dc390_pci_map (PSRB pSRB) @@ -919,40 +927,43 @@ Scsi_Cmnd *pcmd = pSRB->pcmd; struct pci_dev *pdev = pSRB->pSRBDCB->pDCBACB->pdev; dc390_cmd_scp_t* cmdp = ((dc390_cmd_scp_t*)(&pcmd->SCp)); + /* Map sense buffer */ if (pSRB->SRBFlag & AUTO_REQSENSE) { - sg_dma_address(&pSRB->Segmentx) = cmdp->saved_dma_handle = - pci_map_page(pdev, virt_to_page(pcmd->sense_buffer), - (unsigned long)pcmd->sense_buffer & ~PAGE_MASK, sizeof(pcmd->sense_buffer), - DMA_FROM_DEVICE); - sg_dma_len(&pSRB->Segmentx) = sizeof(pcmd->sense_buffer); - pSRB->SGcount = 1; - pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; + pSRB->pSegmentList = dc390_sg_build_single(&pSRB->Segmentx, pcmd->sense_buffer, sizeof(pcmd->sense_buffer)); + pSRB->SGcount = pci_map_sg(pdev, pSRB->pSegmentList, 1, + DMA_FROM_DEVICE); + cmdp->saved_dma_handle = sg_dma_address(pSRB->pSegmentList); + + /* TODO: error handling */ + if (pSRB->SGcount != 1) + error = 1; DEBUG1(printk("%s(): Mapped sense buffer %p at %x\n", __FUNCTION__, pcmd->sense_buffer, cmdp->saved_dma_handle)); - /* Make SG list */ + /* Map SG list */ } else if (pcmd->use_sg) { - pSRB->pSegmentList = (PSGL) pcmd->request_buffer; - pSRB->SGcount = pci_map_sg(pdev, pSRB->pSegmentList, - pcmd->use_sg, - scsi_to_pci_dma_dir(pcmd->sc_data_direction)); + pSRB->pSegmentList = (PSGL) pcmd->request_buffer; + pSRB->SGcount = pci_map_sg(pdev, pSRB->pSegmentList, pcmd->use_sg, + scsi_to_pci_dma_dir(pcmd->sc_data_direction)); /* TODO: error handling */ if (!pSRB->SGcount) error = 1; - DEBUG1(printk("%s(): Mapped SG %p with %d (%d) elements\n", __FUNCTION__, pcmd->request_buffer, pSRB->SGcount, pcmd->use_sg)); + DEBUG1(printk("%s(): Mapped SG %p with %d (%d) elements\n",\ + __FUNCTION__, pcmd->request_buffer, pSRB->SGcount, pcmd->use_sg)); /* Map single segment */ } else if (pcmd->request_buffer && pcmd->request_bufflen) { - sg_dma_address(&pSRB->Segmentx) = cmdp->saved_dma_handle = - pci_map_page(pdev, virt_to_page(pcmd->request_buffer), - (unsigned long)pcmd->request_buffer & ~PAGE_MASK, - pcmd->request_bufflen, scsi_to_pci_dma_dir(pcmd->sc_data_direction)); + pSRB->pSegmentList = dc390_sg_build_single(&pSRB->Segmentx, pcmd->request_buffer, pcmd->request_bufflen); + pSRB->SGcount = pci_map_sg(pdev, pSRB->pSegmentList, 1, + scsi_to_pci_dma_dir(pcmd->sc_data_direction)); + cmdp->saved_dma_handle = sg_dma_address(pSRB->pSegmentList); + /* TODO: error handling */ - sg_dma_len(&pSRB->Segmentx) = pcmd->request_bufflen; - pSRB->SGcount = 1; - pSRB->pSegmentList = (PSGL) &pSRB->Segmentx; + if (pSRB->SGcount != 1) + error = 1; DEBUG1(printk("%s(): Mapped request buffer %p at %x\n", __FUNCTION__, pcmd->request_buffer, cmdp->saved_dma_handle)); /* No mapping !? */ } else pSRB->SGcount = 0; + return error; } @@ -961,21 +972,16 @@ { Scsi_Cmnd* pcmd = pSRB->pcmd; struct pci_dev *pdev = pSRB->pSRBDCB->pDCBACB->pdev; - dc390_cmd_scp_t* cmdp = ((dc390_cmd_scp_t*)(&pcmd->SCp)); + DEBUG1(dc390_cmd_scp_t* cmdp = ((dc390_cmd_scp_t*)(&pcmd->SCp))); if (pSRB->SRBFlag) { - pci_unmap_page(pdev, cmdp->saved_dma_handle, - sizeof(pcmd->sense_buffer), DMA_FROM_DEVICE); + pci_unmap_sg(pdev, &pSRB->Segmentx, 1, DMA_FROM_DEVICE); DEBUG1(printk("%s(): Unmapped sense buffer at %x\n", __FUNCTION__, cmdp->saved_dma_handle)); } else if (pcmd->use_sg) { - pci_unmap_sg(pdev, pcmd->request_buffer, pcmd->use_sg, - scsi_to_pci_dma_dir(pcmd->sc_data_direction)); + pci_unmap_sg(pdev, pcmd->request_buffer, pcmd->use_sg, scsi_to_pci_dma_dir(pcmd->sc_data_direction)); DEBUG1(printk("%s(): Unmapped SG at %p with %d elements\n", __FUNCTION__, pcmd->request_buffer, pcmd->use_sg)); } else if (pcmd->request_buffer && pcmd->request_bufflen) { - pci_unmap_page(pdev, - cmdp->saved_dma_handle, - pcmd->request_bufflen, - scsi_to_pci_dma_dir(pcmd->sc_data_direction)); + pci_unmap_sg(pdev, &pSRB->Segmentx, 1, scsi_to_pci_dma_dir(pcmd->sc_data_direction)); DEBUG1(printk("%s(): Unmapped request buffer at %x\n", __FUNCTION__, cmdp->saved_dma_handle)); } } @@ -1535,15 +1541,16 @@ { PEEprom prom; UCHAR index; - PDCB pDCB, pDCB2; + PDCB pDCB, pDCB2 = 0; pDCB = kmalloc (sizeof(DC390_DCB), GFP_ATOMIC); DCBDEBUG(printk (KERN_INFO "DC390: alloc mem for DCB (ID %i, LUN %i): %p\n", \ id, lun, pDCB)); - + *ppDCB = pDCB; - if (!pDCB) return; - pDCB2 = 0; + if (!pDCB) + return; + if( pACB->DCBCnt == 0 ) { pACB->pLinkDCB = pDCB; @@ -1719,7 +1726,6 @@ pACB->SRBCount = MAX_SRB_CNT; pACB->AdapterIndex = index; pACB->status = 0; - psh->this_id = dc390_eepromBuf[index][EE_ADAPT_SCSI_ID]; pACB->DCBCnt = 0; pACB->TagMaxNum = 2 << dc390_eepromBuf[index][EE_TAG_CMD_NUM]; pACB->ACBFlag = 0; diff -u a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c --- a/drivers/scsi/scsiiom.c Wed 2 Jun 20:22:14 2004 +++ b/drivers/scsi/scsiiom.c Mon 14 Jun 21:20:59 2004 @@ -1364,7 +1364,7 @@ status = pSRB->TargetStatus; ptr = (PSCSI_INQDATA) (pcmd->request_buffer); if( pcmd->use_sg ) - ptr = (PSCSI_INQDATA) sg_dma_address((PSGL) ptr); + ptr = (PSCSI_INQDATA) (page_address(((PSGL) ptr)->page) + ((PSGL) ptr)->offset); DEBUG0(printk (" SRBdone (%02x,%08x), SRB %p, pid %li\n", status, pcmd->result,\ pSRB, pcmd->pid)); ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BK PATCH] SCSI updates for 2.6.6 2004-05-18 20:38 ` James Bottomley 2004-05-19 19:50 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski @ 2004-05-21 1:06 ` Chiaki 1 sibling, 0 replies; 35+ messages in thread From: Chiaki @ 2004-05-21 1:06 UTC (permalink / raw) To: Kurt Garloff Cc: James Bottomley, Guennadi Liakhovetski, Linux SCSI list, Linux kernel list Well this may be a little off topic. But I would like to thank Kurt for supporting DC390 back in 1997 (or 1996? I know it was before the France World Cup soccer games) when I was contemplating of moving to Solaris or linux from OS/2. Without his support to fix some problems (module usage, etc.), I would not have moved to linux at all. The DC390 is still working in my PC box to connect HP DAT2 tape drive and CD/PD combo while faster SCSI disks are connected on other boards. Thank you again, Garloff-san ! PS: This trusty AMD chip board may be still alive in many PC boxes around the world... James Bottomley wrote: > On Tue, 2004-05-18 at 14:47, Guennadi Liakhovetski wrote: > >>Well, Kurt, thanks for the offer. I am prepared and willing to do the work >>on supporting the driver, but I am, perhaps, not skilled enough to be a >>maintainer of a SCSI LDD. My knowledge of the SCSI protocol and the SCSI >>Linux subsystem is pretty limited. On one hand, the driver is little used, >>so, even if I badly break something it is not likely to cause major >>problems. On the other hand, I would feel more comfortable if, at least at >>the beginning, somebody would review my patches. Besides, it would be a >>good opportunity for me to really learn a bit more about SCSI, SCSI Linux >>driver, BIO,... oh well... > > > OK, roll up for me what you'd currently like applied to the driver; > anything that's less than solid, I'd rather mature in the -mm tree for a > while. > > James > > -- int main(void){int j=2003;/*(c)2003 cishikawa. */ char t[] ="<CI> @abcdefghijklmnopqrstuvwxyz.,\n\""; char *i ="g>qtCIuqivb,gCwe\np@.ietCIuqi\"tqkvv is>dnamz"; while(*i)((j+=strchr(t,*i++)-(int)t),(j%=sizeof t-1), (putchar(t[j])));return 0;}/* under GPL */ ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BK PATCH] SCSI updates for 2.6.6 2004-05-18 19:47 ` [BK PATCH] SCSI updates for 2.6.6 Guennadi Liakhovetski 2004-05-18 20:38 ` James Bottomley @ 2004-05-18 22:17 ` Matthew Wilcox 1 sibling, 0 replies; 35+ messages in thread From: Matthew Wilcox @ 2004-05-18 22:17 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Kurt Garloff, James Bottomley, Linux SCSI list, Linux kernel list On Tue, May 18, 2004 at 09:47:56PM +0200, Guennadi Liakhovetski wrote: > Well, Kurt, thanks for the offer. I am prepared and willing to do the work > on supporting the driver, but I am, perhaps, not skilled enough to be a > maintainer of a SCSI LDD. My knowledge of the SCSI protocol and the SCSI > Linux subsystem is pretty limited. On one hand, the driver is little used, > so, even if I badly break something it is not likely to cause major > problems. On the other hand, I would feel more comfortable if, at least at > the beginning, somebody would review my patches. Besides, it would be a > good opportunity for me to really learn a bit more about SCSI, SCSI Linux > driver, BIO,... oh well... Don't worry about it. You'll learn these things in time ... If Kurt's willing to review your patches when you send them, that'd be best (but don't necessarily wait for Kurt's ack before applying). -- "Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2004-06-14 21:48 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20040518184527.GC4859@tpkurt.garloff.de>
2004-05-18 19:47 ` [BK PATCH] SCSI updates for 2.6.6 Guennadi Liakhovetski
2004-05-18 20:38 ` James Bottomley
2004-05-19 19:50 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski
2004-05-19 21:34 ` [PATCH] tmscsim: no internal queue Guennadi Liakhovetski
2004-05-21 15:15 ` [PATCH] tmscsim: 64-bit cleanup James Bottomley
2004-05-21 22:53 ` Guennadi Liakhovetski
2004-05-22 8:09 ` Kurt Garloff
2004-05-22 15:43 ` James Bottomley
2004-05-22 18:10 ` Kurt Garloff
2004-05-22 19:34 ` [PATCH] tmscsim: new interfaces Guennadi Liakhovetski
2004-05-23 10:37 ` Christoph Hellwig
2004-05-23 21:19 ` Guennadi Liakhovetski
2004-05-26 11:45 ` Christoph Hellwig
2004-05-26 19:37 ` [PATCH] fix comment in scsi_host.h Guennadi Liakhovetski
2004-05-26 21:07 ` [PATCH] tmscsim: new interfaces Guennadi Liakhovetski
2004-05-27 21:20 ` Guennadi Liakhovetski
2004-05-28 13:00 ` Christoph Hellwig
2004-05-29 15:36 ` James Bottomley
2004-05-30 21:01 ` [PATCH] tmscsim: Store pDCB in device->hostdata Guennadi Liakhovetski
2004-05-31 9:22 ` Christoph Hellwig
2004-05-31 21:13 ` [PATCH] tmscsim: init / exit cleanup Guennadi Liakhovetski
2004-06-03 13:38 ` Christoph Hellwig
2004-06-03 20:23 ` Guennadi Liakhovetski
2004-06-06 17:09 ` Christoph Hellwig
2004-06-08 6:37 ` Kurt Garloff
2004-05-22 19:38 ` [PATCH] tmscsim: 64-bit cleanup Guennadi Liakhovetski
2004-06-04 22:17 ` Guennadi Liakhovetski
2004-06-05 8:12 ` Guennadi Liakhovetski
2004-06-05 14:02 ` James Bottomley
2004-06-05 20:37 ` Guennadi Liakhovetski
2004-06-05 21:06 ` James Bottomley
2004-06-06 15:06 ` Guennadi Liakhovetski
2004-06-14 21:37 ` Guennadi Liakhovetski
2004-05-21 1:06 ` [BK PATCH] SCSI updates for 2.6.6 Chiaki
2004-05-18 22:17 ` Matthew Wilcox
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox