Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
On 8/4/05, Jesper Juhl <[EMAIL PROTECTED]> wrote:
> On 8/4/05, Saripalli, Venkata Ramanamurthy (STSD) <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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. Ew. Ok, definitly time to stop reading. Dave - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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 pgpuxf9axPChK.pgp Description: PGP signature
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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
pgpJbd386Ly67.pgp
Description: PGP signature
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
On 8/4/05, Saripalli, Venkata Ramanamurthy (STSD) <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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.c2005-07-12 22:52:29.0
> >+0530
> >+++ new/drivers/scsi/cpqfcTScontrol.c2005-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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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.0
>+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
pgpsgMVr5t5t7.pgp
Description: PGP signature
Re: [PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] cpqfc: fix for "Using too much stach" in 2.6 kernel
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 <[EMAIL PROTECTED]>
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.0
+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;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
