Fix cpqfc
cover Fix cpqfc
As there are still people out there trying their luck with the ancient cpqfc driver that is in 2.4 kernel I think we should make this a bit less buggy. The next 2 patches don't change the driver behavior but reduce stack usage and make sure not to copy foreign stack memory.
Eike
PATCH 1/2 Fix cpqfc::cpqfcTSPutLinkQue()
The cpqfcTSPutLinkQue() takes a void* as third argument. Depending on the second argument either a pointer to a struct or to a single integer is passed. In any case 48 DWORDs (192 bytes) of memory are copied. The integer is always located on the stack but nevertheless noone should access memory he doesn't own. The struct copied has a size of only 38 DWORDs, so the copied memory here is also something that does not belong there.
This patch changes the type of the third argument to be TachFCHDR_GCMND* and introduces a new, fourth argument of type integer. Now only the really needed memory is copied. The remaining bytes of the destination buffer are left on their previous (random) values, but they are ignored anyway. If not, this would have lead to problems when random values from the stack had been copied there.
From: Rolf Eike Beer <[email protected]>
drivers/scsi/cpqfcTScontrol.c +6 -7
@@ -645,8 +645,8 @@ int CpqTsProcessIMQEntry(void *host)u16 i, RPCset, DPCset;u32 x_ID;u32 ulBuff, dwStatus; -TachFCHDR_GCMND *fchs;u32 ulFibreFrame[2048 /4 ];// max number of DWORDS in incoming Fibre Frame +TachFCHDR_GCMND *fchs = (TachFCHDR_GCMND *) ulFibreFrame;// cast to examine IB frame u8 ucInboundMessageType;// Inbound CM, dword 3 "type" field ENTER ("ProcessIMQEntry" );
@@ -743,11 +743,11 @@ int CpqTsProcessIMQEntry(void *host)if (!(Exchanges->fcExchange[x_ID].status &DEVICE_REMOVED )) {// presumes device is still there: send ABTS. -cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS , &x_ID); +cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS ,NULL , x_ID); } }else // Abort all other errors { -cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS , &x_ID); +cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS ,NULL , x_ID); }// if the HPE bit is set, we have to CLose the LOOP
@@ -864,7 +864,6 @@ int CpqTsProcessIMQEntry(void *host)// (i.e., Worker) if (ucInboundMessageType ==1 ) { - fchs = (TachFCHDR_GCMND *) ulFibreFrame;// cast to examine IB frame // don't fill up our Q with garbage - only accept FCP-CMND // or XRDY frames if ((fchs->d_id &0xFF000000 ) ==0x06000000 )// CMND
@@ -900,7 +899,7 @@ int CpqTsProcessIMQEntry(void *host) Exchanges->fcExchange[x_ID].status |=SFQ_FRAME ;// presumes device is still there: send ABTS. -cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS , &x_ID); +cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS ,NULL , x_ID);break ;// done } }
@@ -910,7 +909,7 @@ int CpqTsProcessIMQEntry(void *host)else if (ucInboundMessageType ==3 ) {// FC Link Service frames (e.g. PLOGI, ACC) come in here. -cpqfcTSPutLinkQue (cpqfcHBAdata,SFQ_UNKNOWN , ulFibreFrame); +cpqfcTSPutLinkQue (cpqfcHBAdata,SFQ_UNKNOWN , fchs,0 ); }
@@ -1278,7 +1277,7 @@ int CpqTsProcessIMQEntry(void *host)// printk("skipping LINKACTIVE post\n"); }else -cpqfcTSPutLinkQue (cpqfcHBAdata,LINKACTIVE , ulFibreFrame); +cpqfcTSPutLinkQue (cpqfcHBAdata,LINKACTIVE , fchs,0 ); }
drivers/scsi/cpqfcTSinit.c +1 -1
@@ -1295,7 +1295,7 @@ int cpqfcTS_eh_abort(Scsi_Cmnd * Cmnd)// upper layers, we can't make future reference to any of it's // fields (e.g the Nexus). -cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS , &i); +cpqfcTSPutLinkQue (cpqfcHBAdata,BLS_ABTS ,NULL , i);break ; }
drivers/scsi/cpqfcTSstructs.h +1 -1
@@ -955,7 +955,7 @@ PFC_LOGGEDIN_PORT fcFindLoggedInPort(PTA__u8 wwn[8 ],// search linked list for WWN, or... PFC_LOGGEDIN_PORT * pLastLoggedInPort); -void cpqfcTSPutLinkQue (CPQFCHBA * cpqfcHBAdata,int Type,void *QueContent); +void cpqfcTSPutLinkQue (CPQFCHBA * dev,int Type,TachFCHDR_GCMND *fchs,u32 errcode);void fcPutScsiQue (CPQFCHBA * cpqfcHBAdata,int Type,void *QueContent);
drivers/scsi/cpqfcTSworker.c +33 -29
233 lines not on this page · raw message
PATCH 2/2 cpqfc: Reduce stack usage of 2 functions
In reply to Rolf Eike BeerThere are 2 function that do a lookup to the queue and buffer the result. They only care about the first few bytes (to be exactly: 38*sizeof(u32)), but they always pass a buffer of 2kB, which is located on the stack.
This patch reduces this buffer to the really needed size and changes it's type to the correct target type. The rest of the queue is still looked up but the result is ignored (as it would anyway).
There are also some small cleanups for the other arguments of CpqTsGetSFQEntry(), which is the lookup function.
From: Rolf Eike Beer <[email protected]>
drivers/scsi/cpqfcTScontrol.c +25 -31
@@ -50,7 +50,8 @@//#define IMQ_DEBUG 1 static void fcParseLinkStatusCounters (TACHYON * fcChip); -static void CpqTsGetSFQEntry (TACHYON * fcChip,u16 pi,u32 * buffr,u8 UpdateChip); +static void CpqTsGetSFQEntry (PTACHYON fcChip,u32 producerNdx,TachFCHDR_GCMND *fchs, +int UpdateChip);static void cpqfc_free_dma_consistent (CPQFCHBA * cpqfcHBAdata) {
@@ -540,16 +541,13 @@ static int PeekIMQEntry(PTACHYON fcChip,// If we find it, check the incoming frame payload (1st word) // for LILP frame if ((fcChip->IMQ->QEntry[CI ].type &0x1FF ) ==0x104 ) { -TachFCHDR_GCMND *fchs; -u32 ulFibreFrame[2048 /4 ];// max DWORDS in incoming FC Frame -u16 SFQpi = (u16) (fcChip->IMQ->QEntry[CI ].word[0 ] &0x0fffL ); +TachFCHDR_GCMND fchs; +u32 SFQpi = (u32) (fcChip->IMQ->QEntry[CI ].word[0 ] &0x0fffL );CpqTsGetSFQEntry (fcChip, SFQpi,// SFQ producer ndx - ulFibreFrame,// contiguous dest. buffer - FALSE);// DON'T update chip--this is a "lookahead" + &fchs,0 );// DON'T update chip, this is a "lookahead" - fchs = (TachFCHDR_GCMND *) & ulFibreFrame; -if (fchs->pl[0 ] ==ELS_LILP_FRAME ) { +if (fchs.pl[0 ] ==ELS_LILP_FRAME ) {return 1 ;// found the LILP frame! }else {// keep looking...
@@ -645,8 +643,7 @@ int CpqTsProcessIMQEntry(void *host)u16 i, RPCset, DPCset;u32 x_ID;u32 ulBuff, dwStatus; -u32 ulFibreFrame[2048 /4 ];// max number of DWORDS in incoming Fibre Frame -TachFCHDR_GCMND *fchs = (TachFCHDR_GCMND *) ulFibreFrame;// cast to examine IB frame +TachFCHDR_GCMND fchs;u8 ucInboundMessageType;// Inbound CM, dword 3 "type" field ENTER ("ProcessIMQEntry" );
@@ -857,8 +854,8 @@ int CpqTsProcessIMQEntry(void *host)// clears SFQ entry from Tachyon buffer; copies to contiguous ulBuff CpqTsGetSFQEntry (fcChip,// i.e. this Device Object - (u16 ) fcChip->SFQ->producerIndex,// SFQ producer ndx - ulFibreFrame, TRUE);// contiguous destination buffer, update chip + fcChip->SFQ->producerIndex,// SFQ producer ndx + &fchs,1 );// contiguous destination buffer, update chip // analyze the incoming frame outside the INT handler... // (i.e., Worker)
@@ -866,14 +863,14 @@ int CpqTsProcessIMQEntry(void *host)if (ucInboundMessageType ==1 ) {// don't fill up our Q with garbage - only accept FCP-CMND // or XRDY frames -if ((fchs->d_id &0xFF000000 ) ==0x06000000 )// CMND +if ((fchs.d_id &0xFF000000 ) ==0x06000000 )// CMND {// someone sent us a SCSI command // fcPutScsiQue( cpqfcHBAdata, // SFQ_UNASSISTED_FCP, ulFibreFrame); - }else if (((fchs->d_id &0xFF000000 ) ==0x07000000 ) ||// RSP (status) - (fchs->d_id &0xFF000000 ) ==0x05000000 )// XRDY + }else if (((fchs.d_id &0xFF000000 ) ==0x07000000 ) ||// RSP (status) + (fchs.d_id &0xFF000000 ) ==0x05000000 )// XRDY {u32 x_ID;// Unfortunately, ABTS requires a Freeze on the chip so
@@ -891,8 +888,8 @@ int CpqTsProcessIMQEntry(void *host)// Do we have an open exchange that matches this s_id // and ox_id? for (x_ID =0 ; x_ID <TACH_SEST_LEN ; x_ID++) { -if ((fchs->s_id &0xFFFFFF ) == (Exchanges->fcExchange[x_ID].fchs.d_id &0xFFFFFF ) - && (fchs->ox_rx_id &0xFFFF0000 ) == (Exchanges->fcExchange[x_ID].fchs.ox_rx_id &0xFFFF0000 )) { +if ((fchs.s_id &0xFFFFFF ) == (Exchanges->fcExchange[x_ID].fchs.d_id &0xFFFFFF ) + && (fchs.ox_rx_id &0xFFFF0000 ) == (Exchanges->fcExchange[x_ID].fchs.ox_rx_id &0xFFFF0000 )) {// printk(" #R/X frame x_ID %08X# ", fchs->ox_rx_id ); // simulate the anticipated error - since the // SEST was frozen, frames were lost...
@@ -909,7 +906,7 @@ int CpqTsProcessIMQEntry(void *host)else if (ucInboundMessageType ==3 ) {// FC Link Service frames (e.g. PLOGI, ACC) come in here. -cpqfcTSPutLinkQue (cpqfcHBAdata,SFQ_UNKNOWN , fchs,0 ); +cpqfcTSPutLinkQue (cpqfcHBAdata,SFQ_UNKNOWN , &fchs,0 ); }
@@ -1277,7 +1274,7 @@ int CpqTsProcessIMQEntry(void *host)// printk("skipping LINKACTIVE post\n"); }else -cpqfcTSPutLinkQue (cpqfcHBAdata,LINKACTIVE , fchs,0 ); +cpqfcTSPutLinkQue (cpqfcHBAdata,LINKACTIVE , &fchs,0 ); }
@@ -1410,7 +1407,7 @@ int CpqTsProcessIMQEntry(void *host)// to analyze data transfer (successful?), then send a response // frame for this exchange - ulFibreFrame[0 ] = x_ID;// copy for later reference + fchs.reserved = x_ID;// copy for later reference // if this was a TWE, we have to send satus response if (Exchanges->fcExchange[x_ID].type ==SCSI_TWE ) {
@@ -1716,8 +1713,8 @@ int CpqTsDestroyTachLiteQues(void *pHBA,// length from the completion message. The caller passes a buffer large // enough for the complete message (max 2k). -static void CpqTsGetSFQEntry (PTACHYON fcChip,u16 producerNdx,u32 * ulDestPtr,// contiguous destination buffer -u8 UpdateChip) +static void CpqTsGetSFQEntry (PTACHYON fcChip,u32 producerNdx,TachFCHDR_GCMND *fchs, +int UpdateChip) {u32 total_bytes =0 ;u32 consumerIndex = fcChip->SFQ->consumerIndex;
@@ -1727,17 +1724,14 @@ static void CpqTsGetSFQEntry(PTACHYON fc// equal indexes means SFS is copied while (producerNdx != consumerIndex) {// need to process message - total_bytes +=64 ;// maintain count to prevent writing past buffer -// don't allow copies over Fibre Channel defined length! -if (total_bytes <=2048 ) { -memcpy (ulDestPtr, &fcChip->SFQ->QEntry[consumerIndex],64 );// each SFQ entry is 64 bytes - ulDestPtr +=16 ;// advance pointer to next 64 byte block +if (total_bytes <=sizeof (*fchs)) { +memcpy (((void *)fchs) + total_bytes, + &fcChip->SFQ->QEntry[consumerIndex], +min (64 , (int )(sizeof (*fchs) - total_bytes)));// each SFQ entry is 64 bytes } -// Tachyon is producing, -// and we are consuming + total_bytes +=64 ; -if (++consumerIndex >=SFQ_LEN )// check for rollover - consumerIndex =0L ;// reset it + consumerIndex = (consumerIndex +1 ) %SFQ_LEN ; }// if specified, update the Tachlite chip ConsumerIndex...
Re: [PATCH 2.4 2/2] cpqfc: Reduce stack usage of 2 functions
Quoted 10 lines · Rolf Eike Beer
There are 2 function that do a lookup to the queue and buffer the result. They only care about the first few bytes (to be exactly: 38*sizeof(u32)), but they always pass a buffer of 2kB, which is located on the stack.
This patch reduces this buffer to the really needed size and changes it's type to the correct target type. The rest of the queue is still looked up but the result is ignored (as it would anyway).
There are also some small cleanups for the other arguments of
CpqTsGetSFQEntry(), which is the lookup function.
Hi Rolf,
Is there an evidence that the gratuitous stack usage is causing problems with an 8kB stack?
In other ways, this looks more like an optimization than a serious bugfix?
Re: [PATCH 2.4 2/2] cpqfc: Reduce stack usage of 2 functions
Quoted 9 lines · Marcelo Tosatti
Quoted 8 lines · Rolf Eike Beer
There are 2 function that do a lookup to the queue and buffer the result. They only care about the first few bytes (to be exactly: 38*sizeof(u32)), but they always pass a buffer of 2kB, which is located on the stack.
This patch reduces this buffer to the really needed size and changes it's type to the correct target type. The rest of the queue is still looked up but the result is ignored (as it would anyway).
Quoted 7 lines
Quoted 2 lines
There are also some small cleanups for the other arguments of
CpqTsGetSFQEntry(), which is the lookup function.Is there an evidence that the gratuitous stack usage is causing problems with an 8kB stack?
In other ways, this looks more like an optimization than a serious bugfix?
I have no reports of this, but this proves nothing. But why waste one quarter of the stack for nothing?
Eike
Re: [PATCH 2.4 2/2] cpqfc: Reduce stack usage of 2 functions
Quoted 13 lines · Rolf Eike Beer
Quoted 11 lines · Marcelo Tosatti
Quoted 10 lines · Rolf Eike Beer
There are 2 function that do a lookup to the queue and buffer the result. They only care about the first few bytes (to be exactly: 38*sizeof(u32)), but they always pass a buffer of 2kB, which is located on the stack.
This patch reduces this buffer to the really needed size and changes it's type to the correct target type. The rest of the queue is still looked up but the result is ignored (as it would anyway).
There are also some small cleanups for the other arguments of
CpqTsGetSFQEntry(), which is the lookup function.
Quoted 7 lines
Quoted 4 lines
Is there an evidence that the gratuitous stack usage is causing problems with an 8kB stack?
In other ways, this looks more like an optimization than a serious bugfix?
I have no reports of this, but this proves nothing. But why waste one quarter of the stack for nothing?
My point is that v2.4 is into deep maintenance mode, which means that only critical bug fixes get merged into the tree.
I'm not against the patch per se.
Re: [PATCH 2.4 2/2] cpqfc: Reduce stack usage of 2 functions
Am Dienstag, 3. Januar 2006 12:59 schrieben Sie:
Quoted 18 lines · Rolf Eike Beer
Quoted 17 lines
Quoted 16 lines · Marcelo Tosatti
Quoted 11 lines · Rolf Eike Beer
There are 2 function that do a lookup to the queue and buffer the result. They only care about the first few bytes (to be exactly:
38*sizeof(u32)), but they always pass a buffer of 2kB, which is located on the stack.This patch reduces this buffer to the really needed size and changes it's type to the correct target type. The rest of the queue is still looked up but the result is ignored (as it would anyway).
There are also some small cleanups for the other arguments of
CpqTsGetSFQEntry(), which is the lookup function.Is there an evidence that the gratuitous stack usage is causing problems with an 8kB stack?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
In other ways, this looks more like an optimization than a serious bugfix?
I have no reports of this, but this proves nothing. But why waste one quarter of the stack for nothing?
My point is that v2.4 is into deep maintenance mode, which means that only critical bug fixes get merged into the tree.
Doesn't "ugly like hell" count as critical? *g*
Quoted 1 line
I'm not against the patch per se.
I've looked on it. Both of the functions with the 2k+ stack usage are called from interrupt handler, one directly and one from the other. The stack usage at this point is little above 4k plus everything that is needed to call the interrupt handler. I don't know if this must be considered harmful.
Eike