Skip to contentSkip to conversation

2.6.6 invalid usage of GFP_DMA in drivers/scsi/pluto.c

PATCH In review Yury Umanets 2 participants 3 messages Jun 8, 2004

lore
Share & tools
lore

Yury Umanets

author
#0 lore raw

PATCH 1/1 2.6.6 invalid usage of GFP_DMA in drivers/scsi/pluto.c

Hello Andrew, guys,

Found this, what seems to be an invalid usage of GFP_DMA flag. Is this patch okay?

Thanks.

 ./linux-2.6.6-modified/drivers/scsi/pluto.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)
Signed-off-by Yury Umanets <[email protected]>
linux-2.6.6-modified/drivers/scsi/pluto.c renamed from linux-2.6.6/drivers/scsi/pluto.c +0 -1
@@ -117,7 +117,8 @@ int __init pluto_detect(Scsi_Host_Templa
 #endif
 			return 0;
 	}
-	fcs = (struct ctrl_inquiry *) kmalloc (sizeof (struct ctrl_inquiry) *

fcscount, GFP_DMA);
+ fcs = (struct ctrl_inquiry *) kmalloc (sizeof (struct ctrl_inquiry) *
fcscount,
+ GFP_KERNEL | GFP_DMA);
if (!fcs) {
printk ("PLUTO: Not enough memory to probe\n"); return 0;

Signature · 1 line
umka
#1 lore raw
Quoted 5 lines · Yury Umanets

Hello Andrew, guys,

Found this, what seems to be an invalid usage of GFP_DMA flag. Is this patch okay?

Nope.

GFP_DMA means "from the lower 16MB of memory". It's needed for crufty old eisa hardware which only does 24-bit DMA. It's meaningless to OR this with GFP_KERNEL.

However it's a bit odd that GFP_DMA implies !__GFP_WAIT. It would be valid to hunt down GFP_DMA users who should really be using GFP_DMA|__GFP_WAIT, but this stuff is so old and crufty I'd be inclined to leave it all alone.

Quoted 14 lines
linux-2.6.6-modified/drivers/scsi/pluto.c renamed from linux-2.6.6/drivers/scsi/pluto.c +2 -1
@@ -117,7 +117,8 @@ int __init pluto_detect(Scsi_Host_Templa
 #endif
 			return 0;
 	}
-	fcs = (struct ctrl_inquiry *) kmalloc (sizeof (struct ctrl_inquiry) *
 fcscount, GFP_DMA);
+	fcs = (struct ctrl_inquiry *) kmalloc (sizeof (struct ctrl_inquiry) *
 fcscount, 
+			GFP_KERNEL | GFP_DMA);
 	if (!fcs) {

linux-2.6.6-modified/drivers/scsi/pluto.c

Quoted 2 lines
linux-2.6.6-modified/drivers/scsi/pluto.c renamed from linux-2.6.6/drivers/scsi/pluto.c +0 -0
 		printk ("PLUTO: Not enough memory to probe\n");
 		return 0;

Your patch is wordwrapped and uses weird headers (please omit the leading ./ from the pathnames).

author
#2 lore raw
Quoted 11 lines · Andrew Morton
Quoted 5 lines · Yury Umanets

Hello Andrew, guys,

Found this, what seems to be an invalid usage of GFP_DMA flag. Is this patch okay?

Nope.

GFP_DMA means "from the lower 16MB of memory". It's needed for crufty old eisa hardware which only does 24-bit DMA.

This is clear. And sure, it shows that caller wants some memory from DMA zone if it is possible.

What I mean is that, GFP_DMA seems to be not full specifier and only flag which can be used along with something else.

Say gfp mask roughly consist of two kinds on instructions:
* what memory zone to use - zone specifier.
* how to behave during allocation (IO, sleep, etc) - behavior specifier.

My concern is will it behave correctly with only GFP_DMA? It seems to behave like do not use emergency pools and do not wait at the same time what is risky. Is that right?

Quoted 6 lines

It's meaningless to OR this with
GFP_KERNEL.

However it's a bit odd that GFP_DMA implies !__GFP_WAIT. It would be valid to hunt down GFP_DMA users who should really be using GFP_DMA|__GFP_WAIT,

Seems that all GFP_DMA users use it with __GFP_WAIT or GFP_KERNEL. And I'd prefer to add __GFP_WAIT here also.

Quoted 15 lines

but this stuff is so old and crufty I'd be inclined to leave it all alone.

Quoted 12 lines
linux-2.6.6-modified/drivers/scsi/pluto.c renamed from linux-2.6.6/drivers/scsi/pluto.c +2 -1
@@ -117,7 +117,8 @@ int __init pluto_detect(Scsi_Host_Templa
 #endif
 			return 0;
 	}
-	fcs = (struct ctrl_inquiry *) kmalloc (sizeof (struct ctrl_inquiry) *
 fcscount, GFP_DMA);
+	fcs = (struct ctrl_inquiry *) kmalloc (sizeof (struct ctrl_inquiry) *
 fcscount, 
+			GFP_KERNEL | GFP_DMA);
Quoted 8 lines

linux-2.6.6-modified/drivers/scsi/pluto.c

Quoted 4 lines
linux-2.6.6-modified/drivers/scsi/pluto.c renamed from linux-2.6.6/drivers/scsi/pluto.c +0 -0
 	if (!fcs) {
linux-2.6.6-modified/drivers/scsi/pluto.c renamed from linux-2.6.6/drivers/scsi/pluto.c +0 -0
 		printk ("PLUTO: Not enough memory to probe\n");
 		return 0;

Your patch is wordwrapped and uses weird headers (please omit the leading ./ from the pathnames).

Sorry, next will use another mailer.

Signature · 1 line
umka