* [PATCH] Remove unnecessary kmalloc/kfree calls in mtdchar
@ 2006-04-15 2:38 Thiago Galesi
2006-04-17 12:06 ` Josh Boyer
0 siblings, 1 reply; 7+ messages in thread
From: Thiago Galesi @ 2006-04-15 2:38 UTC (permalink / raw)
To: David Woodhouse, linux-kernel, linux-mtd
This patch removes the use of repeated calls to kmalloc / kfree when
writing / reading from a MTD char device. Not the ideal solution
mentioned in the driver, but nonetheless better.
Signed-off by Thiago Galesi <thiagogalesi@gmail.com>
---
(Please CC me as I'm not subscribed to linux-mtd)
Index: linux-2.6.16.2/drivers/mtd/mtdchar.c
===================================================================
--- linux-2.6.16.2.orig/drivers/mtd/mtdchar.c
+++ linux-2.6.16.2/drivers/mtd/mtdchar.c
@@ -170,15 +170,18 @@ static ssize_t mtd_read(struct file *fil
/* FIXME: Use kiovec in 2.5 to lock down the user's buffers
and pass them directly to the MTD functions */
- while (count) {
- if (count > MAX_KMALLOC_SIZE)
- len = MAX_KMALLOC_SIZE;
- else
- len = count;
- kbuf=kmalloc(len,GFP_KERNEL);
- if (!kbuf)
- return -ENOMEM;
+ if (count > MAX_KMALLOC_SIZE)
+ len = MAX_KMALLOC_SIZE;
+ else
+ len = count;
+
+ kbuf=kmalloc(len,GFP_KERNEL);
+
+ if (!kbuf)
+ return -ENOMEM;
+
+ while (count) {
switch (MTD_MODE(file)) {
case MTD_MODE_OTP_FACT:
@@ -215,9 +218,9 @@ static ssize_t mtd_read(struct file *fil
return ret;
}
- kfree(kbuf);
}
+ kfree(kbuf);
return total_retlen;
} /* mtd_read */
@@ -241,17 +244,18 @@ static ssize_t mtd_write(struct file *fi
if (!count)
return 0;
- while (count) {
- if (count > MAX_KMALLOC_SIZE)
- len = MAX_KMALLOC_SIZE;
- else
- len = count;
+ if (count > MAX_KMALLOC_SIZE)
+ len = MAX_KMALLOC_SIZE;
+ else
+ len = count;
+
+ kbuf=kmalloc(len,GFP_KERNEL);
+ if (!kbuf) {
+ printk("kmalloc is null\n");
+ return -ENOMEM;
+ }
- kbuf=kmalloc(len,GFP_KERNEL);
- if (!kbuf) {
- printk("kmalloc is null\n");
- return -ENOMEM;
- }
+ while (count) {
if (copy_from_user(kbuf, buf, len)) {
kfree(kbuf);
@@ -282,10 +286,9 @@ static ssize_t mtd_write(struct file *fi
kfree(kbuf);
return ret;
}
-
- kfree(kbuf);
}
+ kfree(kbuf);
return total_retlen;
} /* mtd_write */
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Remove unnecessary kmalloc/kfree calls in mtdchar
2006-04-15 2:38 [PATCH] Remove unnecessary kmalloc/kfree calls in mtdchar Thiago Galesi
@ 2006-04-17 12:06 ` Josh Boyer
2006-04-17 13:19 ` Thiago Galesi
0 siblings, 1 reply; 7+ messages in thread
From: Josh Boyer @ 2006-04-17 12:06 UTC (permalink / raw)
To: Thiago Galesi; +Cc: David Woodhouse, linux-kernel, linux-mtd
On 4/14/06, Thiago Galesi <thiagogalesi@gmail.com> wrote:
> This patch removes the use of repeated calls to kmalloc / kfree when
> writing / reading from a MTD char device. Not the ideal solution
> mentioned in the driver, but nonetheless better.
NAK. This patch introduces a bug. See below.
>
> Index: linux-2.6.16.2/drivers/mtd/mtdchar.c
> ===================================================================
> --- linux-2.6.16.2.orig/drivers/mtd/mtdchar.c
> +++ linux-2.6.16.2/drivers/mtd/mtdchar.c
> @@ -170,15 +170,18 @@ static ssize_t mtd_read(struct file *fil
>
> /* FIXME: Use kiovec in 2.5 to lock down the user's buffers
> and pass them directly to the MTD functions */
> - while (count) {
> - if (count > MAX_KMALLOC_SIZE)
> - len = MAX_KMALLOC_SIZE;
> - else
> - len = count;
>
> - kbuf=kmalloc(len,GFP_KERNEL);
> - if (!kbuf)
> - return -ENOMEM;
> + if (count > MAX_KMALLOC_SIZE)
> + len = MAX_KMALLOC_SIZE;
> + else
> + len = count;
Now that len is set outside of the loop, it is always the same size.
If count is large enough to require more than a single read, the the
original size will still be used and it could overflow the user's
buffer.
I agree that doing the kmallocs in a loop looks nasty. But we need to
make sure moving out of the loop doesn't break things.
josh
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] Remove unnecessary kmalloc/kfree calls in mtdchar
2006-04-17 12:06 ` Josh Boyer
@ 2006-04-17 13:19 ` Thiago Galesi
2006-04-17 14:50 ` David Woodhouse
0 siblings, 1 reply; 7+ messages in thread
From: Thiago Galesi @ 2006-04-17 13:19 UTC (permalink / raw)
To: Josh Boyer; +Cc: David Woodhouse, linux-kernel
Josh
Thank you for your feedback! You're right, and both read and write had
the problem. It's probably better to kmalloc once but calculate len in
the old way. Hopefully I got it right this time.
Index: linux-2.6.16.2/drivers/mtd/mtdchar.c
===================================================================
--- linux-2.6.16.2.orig/drivers/mtd/mtdchar.c
+++ linux-2.6.16.2/drivers/mtd/mtdchar.c
@@ -170,16 +170,22 @@ static ssize_t mtd_read(struct file *fil
/* FIXME: Use kiovec in 2.5 to lock down the user's buffers
and pass them directly to the MTD functions */
+
+ if (count > MAX_KMALLOC_SIZE)
+ kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
+ else
+ kbuf=kmalloc(count, GFP_KERNEL);
+
+ if (!kbuf)
+ return -ENOMEM;
+
while (count) {
+
if (count > MAX_KMALLOC_SIZE)
len = MAX_KMALLOC_SIZE;
else
len = count;
- kbuf=kmalloc(len,GFP_KERNEL);
- if (!kbuf)
- return -ENOMEM;
-
switch (MTD_MODE(file)) {
case MTD_MODE_OTP_FACT:
ret = mtd->read_fact_prot_reg(mtd, *ppos, len, &retlen, kbuf);
@@ -215,9 +221,9 @@ static ssize_t mtd_read(struct file *fil
return ret;
}
- kfree(kbuf);
}
+ kfree(kbuf);
return total_retlen;
} /* mtd_read */
@@ -241,18 +247,23 @@ static ssize_t mtd_write(struct file *fi
if (!count)
return 0;
+ if (count > MAX_KMALLOC_SIZE)
+ kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
+ else
+ kbuf=kmalloc(count, GFP_KERNEL);
+
+ if (!kbuf) {
+ printk("kmalloc is null\n");
+ return -ENOMEM;
+ }
+
while (count) {
+
if (count > MAX_KMALLOC_SIZE)
len = MAX_KMALLOC_SIZE;
else
len = count;
- kbuf=kmalloc(len,GFP_KERNEL);
- if (!kbuf) {
- printk("kmalloc is null\n");
- return -ENOMEM;
- }
-
if (copy_from_user(kbuf, buf, len)) {
kfree(kbuf);
return -EFAULT;
@@ -282,10 +293,9 @@ static ssize_t mtd_write(struct file *fi
kfree(kbuf);
return ret;
}
-
- kfree(kbuf);
}
+ kfree(kbuf);
return total_retlen;
} /* mtd_write */
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Remove unnecessary kmalloc/kfree calls in mtdchar
2006-04-17 13:19 ` Thiago Galesi
@ 2006-04-17 14:50 ` David Woodhouse
2006-04-17 15:03 ` Thiago Galesi
0 siblings, 1 reply; 7+ messages in thread
From: David Woodhouse @ 2006-04-17 14:50 UTC (permalink / raw)
To: Thiago Galesi; +Cc: Josh Boyer, linux-kernel
On Mon, 2006-04-17 at 10:19 -0300, Thiago Galesi wrote:
>
> + if (!kbuf) {
> + printk("kmalloc is null\n");
> + return -ENOMEM;
> + }
Don't printk -- especially without a priority. Either just bail out with
-ENOMEM or try a smaller size.
--
dwmw2
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] Remove unnecessary kmalloc/kfree calls in mtdchar
2006-04-17 14:50 ` David Woodhouse
@ 2006-04-17 15:03 ` Thiago Galesi
[not found] ` <1145290249.13200.17.camel@pmac.infradead.org>
0 siblings, 1 reply; 7+ messages in thread
From: Thiago Galesi @ 2006-04-17 15:03 UTC (permalink / raw)
To: David Woodhouse; +Cc: Josh Boyer, linux-kernel
Silly me. Ok, the original had it already, but it has to go...
On 4/17/06, David Woodhouse <dwmw2@infradead.org> wrote:
> Don't printk -- especially without a priority. Either just bail out with
> -ENOMEM or try a smaller size.
>
Index: linux-2.6.16.2/drivers/mtd/mtdchar.c
===================================================================
--- linux-2.6.16.2.orig/drivers/mtd/mtdchar.c
+++ linux-2.6.16.2/drivers/mtd/mtdchar.c
@@ -170,16 +170,22 @@ static ssize_t mtd_read(struct file *fil
/* FIXME: Use kiovec in 2.5 to lock down the user's buffers
and pass them directly to the MTD functions */
+
+ if (count > MAX_KMALLOC_SIZE)
+ kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
+ else
+ kbuf=kmalloc(count, GFP_KERNEL);
+
+ if (!kbuf)
+ return -ENOMEM;
+
while (count) {
+
if (count > MAX_KMALLOC_SIZE)
len = MAX_KMALLOC_SIZE;
else
len = count;
- kbuf=kmalloc(len,GFP_KERNEL);
- if (!kbuf)
- return -ENOMEM;
-
switch (MTD_MODE(file)) {
case MTD_MODE_OTP_FACT:
ret = mtd->read_fact_prot_reg(mtd, *ppos, len, &retlen, kbuf);
@@ -215,9 +221,9 @@ static ssize_t mtd_read(struct file *fil
return ret;
}
- kfree(kbuf);
}
+ kfree(kbuf);
return total_retlen;
} /* mtd_read */
@@ -241,18 +247,21 @@ static ssize_t mtd_write(struct file *fi
if (!count)
return 0;
+ if (count > MAX_KMALLOC_SIZE)
+ kbuf=kmalloc(MAX_KMALLOC_SIZE, GFP_KERNEL);
+ else
+ kbuf=kmalloc(count, GFP_KERNEL);
+
+ if (!kbuf)
+ return -ENOMEM;
+
while (count) {
+
if (count > MAX_KMALLOC_SIZE)
len = MAX_KMALLOC_SIZE;
else
len = count;
- kbuf=kmalloc(len,GFP_KERNEL);
- if (!kbuf) {
- printk("kmalloc is null\n");
- return -ENOMEM;
- }
-
if (copy_from_user(kbuf, buf, len)) {
kfree(kbuf);
return -EFAULT;
@@ -282,10 +291,9 @@ static ssize_t mtd_write(struct file *fi
kfree(kbuf);
return ret;
}
-
- kfree(kbuf);
}
+ kfree(kbuf);
return total_retlen;
} /* mtd_write */
Signed-off-by Thiago Galesi <thiagogalesi@gmail.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2006-04-18 2:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-04-15 2:38 [PATCH] Remove unnecessary kmalloc/kfree calls in mtdchar Thiago Galesi
2006-04-17 12:06 ` Josh Boyer
2006-04-17 13:19 ` Thiago Galesi
2006-04-17 14:50 ` David Woodhouse
2006-04-17 15:03 ` Thiago Galesi
[not found] ` <1145290249.13200.17.camel@pmac.infradead.org>
2006-04-17 16:24 ` Thiago Galesi
2006-04-18 2:06 ` Josh Boyer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®