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