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

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

[Saripalli, Venkata Ramanamurthy (STSD)]

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.

Please consider this for inclusion

Signed-off-by: Ramanamurthy Saripalli <saripalli@hp.com>

 cpqfcTScontrol.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
------------------------------------------------------------------------
-------
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);
 
+	  ulFibreFrame = kmalloc((2048/4), GFP_KERNEL);
+	
 	  CpqTsGetSFQEntry( fcChip,
             SFQpi,        // SFQ producer ndx         
 	    ulFibreFrame, // contiguous dest. buffer
 	    FALSE);       // DON'T update chip--this is a "lookahead"
           
-	  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...
 	  }
 	}  
@@ -718,12 +721,12 @@ int CpqTsProcessIMQEntry(void *host)
   ULONG x_ID;
   ULONG ulBuff, dwStatus;
   TachFCHDR_GCMND* fchs;
-#error This is too much stack
-  ULONG ulFibreFrame[2048/4];  // max number of DWORDS in incoming
Fibre Frame
+  ULONG *ulFibreFrame;  // max number of DWORDS in incoming Fibre Frame
   UCHAR ucInboundMessageType;  // Inbound CM, dword 3 "type" field
 
   ENTER("ProcessIMQEntry");
    
+  ulFibreFrame = kmalloc((2048/4), GFP_KERNEL); 	 
 
 				// check TachLite's IMQ producer index -
 				// is a new message waiting for us?
@@ -1627,6 +1630,7 @@ int CpqTsProcessIMQEntry(void *host)
 
   LEAVE("ProcessIMQEntry");
   
+  kfree(ulFibreFrame);	 
   return iStatus;
 }
 

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

[Dave Jones]

On Thu, Aug 04, 2005 at 10:09:51AM +0530, Saripalli, Venkata Ramanamurthy (STSD) wrote:
 > -          ULONG ulFibreFrame[2048/4];  // max DWORDS in incoming FC

This is 512 ulongs, which is 2KB.

 > +	  ulFibreFrame = kmalloc((2048/4), GFP_KERNEL);

You're replacing it with a 512 byte buffer.

		Dave

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

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]

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 #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Denis Vlasenko]

On Thursday 04 August 2005 12:38, Rolf Eike Beer wrote:
> 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)].

Also you need to check for NULL return.
--
vda 

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

[Dave Jones]

On Thu, Aug 04, 2005 at 11:38:30AM +0200, Rolf Eike Beer wrote:
 > 
 > >+	  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)].

wasteful. We only ever use 2048 bytes of this array, so doubling
its size on 64bit is pointless, unless you make changes later on
in the driver. (Which I think don't make sense, as we just copy
32 64byte chunks).

Ermm, actually this looks totally bogus..
CpqTsGetSFQEntry() ...

    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
    }

we're trashing the last 48 bytes of every copy we make.
Does this driver even work ?

		Dave

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

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

Am Donnerstag, 4. August 2005 17:40 schrieb Dave Jones:
>On Thu, Aug 04, 2005 at 11:38:30AM +0200, Rolf Eike Beer wrote:
> > >+	  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)].
>
>wasteful. We only ever use 2048 bytes of this array, so doubling
>its size on 64bit is pointless, unless you make changes later on
>in the driver. (Which I think don't make sense, as we just copy
>32 64byte chunks).

No, this is how it should have been before. This way it would have been clear 
where the magic 4 came from.

>Ermm, actually this looks totally bogus..
>CpqTsGetSFQEntry() ...
>
>    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
>    }
>
>we're trashing the last 48 bytes of every copy we make.
>Does this driver even work ?

No, ulDestPtr ist ULONG* so we increase it by sizeof(ULONG)*16 which is 64. 
This is one of the places I was talking about where people might miss what's 
going on. ;) IMHO it makes absolutely no sense to use a ULONG* at this place.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Dave Jones]

On Thu, Aug 04, 2005 at 05:56:14PM +0200, Rolf Eike Beer wrote:
 > Am Donnerstag, 4. August 2005 17:40 schrieb Dave Jones:
 > >On Thu, Aug 04, 2005 at 11:38:30AM +0200, Rolf Eike Beer wrote:
 > > > >+	  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)].
 > >
 > >wasteful. We only ever use 2048 bytes of this array, so doubling
 > >its size on 64bit is pointless, unless you make changes later on
 > >in the driver. (Which I think don't make sense, as we just copy
 > >32 64byte chunks).
 > 
 > No, this is how it should have been before. This way it would have been clear 
 > where the magic 4 came from.

It's pointless to fix this, without fixing also CpqTsGetSFQEntry()
...

 > >we're trashing the last 48 bytes of every copy we make.
 > >Does this driver even work ?
 > 
 > No, ulDestPtr ist ULONG* so we increase it by sizeof(ULONG)*16 which is 64. 

Duh, yes.  That is broken on 64-bit however, where it will advance 128 bytes
instead of 64 bytes.

		Dave

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

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

Dave Jones wrote:
>On Thu, Aug 04, 2005 at 05:56:14PM +0200, Rolf Eike Beer wrote:
> > Am Donnerstag, 4. August 2005 17:40 schrieb Dave Jones:
> > >On Thu, Aug 04, 2005 at 11:38:30AM +0200, Rolf Eike Beer wrote:
> > > > >+	  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)].
> > >
> > >wasteful. We only ever use 2048 bytes of this array, so doubling
> > >its size on 64bit is pointless, unless you make changes later on
> > >in the driver. (Which I think don't make sense, as we just copy
> > >32 64byte chunks).
> >
> > No, this is how it should have been before. This way it would have been
> > clear where the magic 4 came from.
>
>It's pointless to fix this, without fixing also CpqTsGetSFQEntry()
>...

At least half of the file should be rewritten.

> > >we're trashing the last 48 bytes of every copy we make.
> > >Does this driver even work ?
> >
> > No, ulDestPtr ist ULONG* so we increase it by sizeof(ULONG)*16 which is
> > 64.
>
>Duh, yes.  That is broken on 64-bit however, where it will advance 128 bytes
>instead of 64 bytes.

ULONG is defined to __u32 in some of the cpq* headers.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Dave Jones]

On Thu, Aug 04, 2005 at 07:11:38PM +0200, Rolf Eike Beer wrote:
 > >It's pointless to fix this, without fixing also CpqTsGetSFQEntry()
 > At least half of the file should be rewritten.

Just half ? You're such an optimist :-)

 > > > No, ulDestPtr ist ULONG* so we increase it by sizeof(ULONG)*16 which is
 > > > 64.
 > >Duh, yes.  That is broken on 64-bit however, where it will advance 128 bytes
 > >instead of 64 bytes.
 > 
 > ULONG is defined to __u32 in some of the cpq* headers.

Ewwwww.
Ok, definitly time to stop reading.

		Dave

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

[Jesper Juhl]

On 8/4/05, Saripalli, Venkata Ramanamurthy (STSD) <saripalli@hp.com> 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.
> 
[snip]
>            if( fchs->pl[0] == ELS_LILP_FRAME)
>           {
> +           kfree(ulFibreFrame);
>              return 1; // found the LILP frame!
>           }
>           else
>           {
> +           kfree(ulFibreFrame);
>             // keep looking...
>           }

The first thing you do in either branch is to call
kfree(ulFibreFrame); , so instead of having the call in both branches
you might as well just have one call before the if().  Ohh and this
looks like it could do with a CodingStyle cleanup as well.

kfree(ulFibreFrame);
if (fchs->pl[0] == ELS_LILP_FRAME)
        return 1; /* found the LILP frame! */
/* keep looking */


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

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

[Jesper Juhl]

On 8/4/05, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> On 8/4/05, Saripalli, Venkata Ramanamurthy (STSD) <saripalli@hp.com> 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.
> >
> [snip]
> >            if( fchs->pl[0] == ELS_LILP_FRAME)
> >           {
> > +           kfree(ulFibreFrame);
> >              return 1; // found the LILP frame!
> >           }
> >           else
> >           {
> > +           kfree(ulFibreFrame);
> >             // keep looking...
> >           }
> 
> The first thing you do in either branch is to call
> kfree(ulFibreFrame); , so instead of having the call in both branches
> you might as well just have one call before the if().  Ohh and this
> looks like it could do with a CodingStyle cleanup as well.
> 
> kfree(ulFibreFrame);
> if (fchs->pl[0] == ELS_LILP_FRAME)
>         return 1; /* found the LILP frame! */
> /* keep looking */

Whoops, as Rolf Eike Beer pointed out to me, I snipped one line too many. 
  fchs = (TachFCHDR_GCMND*)ulFibreFrame;
So, the kfree inside each branch is correct. Freeing it just before
the if would be wrong.
Sorry about that.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html