Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

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