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
Attachment:
pgpgrVwzInffx.pgp
Description: PGP signature
