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/drivers/scsi/pluto.c Mon May 10 05:32:27 2004
+++ ./linux-2.6.6-modified/drivers/scsi/pluto.c Tue Jun 8 11:26:07 2004
@@ -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;
--
umka
Yury Umanets <[email protected]> wrote:
>
> 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.
>
> --- ./linux-2.6.6/drivers/scsi/pluto.c Mon May 10 05:32:27 2004
> +++ ./linux-2.6.6-modified/drivers/scsi/pluto.c Tue Jun 8 11:26:07 2004
> @@ -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;
>
Your patch is wordwrapped and uses weird headers (please omit the leading
./ from the pathnames).
On Tue, 2004-06-08 at 13:17, Andrew Morton wrote:
> Yury Umanets <[email protected]> wrote:
> >
> > 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?
> 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.
> but this stuff is so old and crufty I'd be inclined to leave it all alone.
>
>
> >
> > --- ./linux-2.6.6/drivers/scsi/pluto.c Mon May 10 05:32:27 2004
> > +++ ./linux-2.6.6-modified/drivers/scsi/pluto.c Tue Jun 8 11:26:07 2004
> > @@ -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;
> >
>
> Your patch is wordwrapped and uses weird headers (please omit the leading
> ./ from the pathnames).
Sorry, next will use another mailer.
--
umka