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

Rolf Eike Beer
2005-08-04T09:32:09Z

[-- 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 --]