Messages in this thread | | | From | Rolf Eike Beer <> | Subject | Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel | Date | Thu, 4 Aug 2005 11:38:30 +0200 |
| |
Saripalli, Venkata Ramanamurthy (STSD) wrote: >Patch 1 of 2 > >This patch fixes the "#error this is too much stack" in 2.6 kernel. >Using kmalloc to allocate memory to ulFibreFrame.
Good idea.
>Please consider this for inclusion
Your patch is line-wrapped and can't be applied. Your second patch is also line wrapped. And it touches this file in a different way so they can't be applied cleanly over each other.
>diff -burpN old/drivers/scsi/cpqfcTScontrol.c >new/drivers/scsi/cpqfcTScontrol.c >--- old/drivers/scsi/cpqfcTScontrol.c 2005-07-12 22:52:29.000000000 >+0530 >+++ new/drivers/scsi/cpqfcTScontrol.c 2005-07-18 22:19:54.229947176 >+0530 >@@ -606,22 +606,25 @@ static int PeekIMQEntry( PTACHYON fcChip > if( (fcChip->IMQ->QEntry[CI].type & 0x1FF) == 0x104 ) > { > TachFCHDR_GCMND* fchs; >-#error This is too much stack >- ULONG ulFibreFrame[2048/4]; // max DWORDS in incoming FC >Frame >+ ULONG *ulFibreFrame; // max DWORDS in incoming FC Frame > USHORT SFQpi = (USHORT)(fcChip->IMQ->QEntry[CI].word[0] & >0x0fffL);
Why not use a void* here as type for the buffer? Or even better: remove this at all and directly use fchs as target, because this is the only place where this buffer goes to?
>+ ulFibreFrame = kmalloc((2048/4), GFP_KERNEL);
The size bug was already found by Dave Jones. This never should be written this way (not your fault). The array should have been [2048/sizeof(ULONG)].
> CpqTsGetSFQEntry( fcChip, > SFQpi, // SFQ producer ndx > ulFibreFrame, // contiguous dest. buffer > FALSE); // DON'T update chip--this is a "lookahead"
CpqTsGetSFQEntry() should be modified to take a void* as third argument IMHO.
>- fchs = (TachFCHDR_GCMND*)&ulFibreFrame; >+ fchs = (TachFCHDR_GCMND*)ulFibreFrame; > if( fchs->pl[0] == ELS_LILP_FRAME) > { >+ kfree(ulFibreFrame); > return 1; // found the LILP frame! > } > else > { >+ kfree(ulFibreFrame); > // keep looking... > } > }
What a ...
I would prefer if someone goes and really cleans up this driver.
-read Documentation/Codingstyle -go through Lindent. -kill this ULONG stuff. If you want __u32 use it. -use void* for "just a buffer" -don't use hardcoded type sizes. Use sizeof(type) to make clear what kind of magic is going on. -this is C, not C++. No C++ comments, use fewer uppercase letters.
The way it is is very likely to cause people missing what's really going on at some places, which will cause errors afterwards.
Eike [unhandled content-type:application/pgp-signature] | |