[PATCH 1/2] regmap: debugfs: remove bogus check

[PATCH 1/2] regmap: debugfs: remove bogus check

[Rasmus Villemoes]

snprintf cannot return a negative value (unless map->dev->driver->name
happens to be over 2G long). So remove the bogus check.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/base/regmap/regmap-debugfs.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 4a4737887cce..9fb3f6fc26b0 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -48,10 +48,6 @@ static ssize_t regmap_name_read_file(struct file *file,
 		return -ENOMEM;
 
 	ret = snprintf(buf, PAGE_SIZE, "%s\n", map->dev->driver->name);
-	if (ret < 0) {
-		kfree(buf);
-		return ret;
-	}
 
 	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
 	kfree(buf);
-- 
2.1.3

[PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read_file()

[Rasmus Villemoes]

* A page is a bit much for two integers and a bit of punctuation. 64
  bytes should suffice.

* Calling strlen() on entry no less than three times is silly,
  especially when snprintf() has returned that value (which was just
  thrown away).

* Transferring entry to the output buffer using snprintf is silly,
  when we know the length. Use memcpy instead.

* Making the newline part of entry, we can avoid having to account for
  it separately in multiple places.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/base/regmap/regmap-debugfs.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 9fb3f6fc26b0..01110c0b8be0 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -333,6 +333,8 @@ static ssize_t regmap_reg_ranges_read_file(struct file *file,
 	char *buf;
 	char *entry;
 	int ret;
+	unsigned entry_len;
+	const unsigned entry_size = 64;
 
 	if (*ppos < 0 || !count)
 		return -EINVAL;
@@ -341,7 +343,7 @@ static ssize_t regmap_reg_ranges_read_file(struct file *file,
 	if (!buf)
 		return -ENOMEM;
 
-	entry = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	entry = kmalloc(entry_size, GFP_KERNEL);
 	if (!entry) {
 		kfree(buf);
 		return -ENOMEM;
@@ -360,18 +362,15 @@ static ssize_t regmap_reg_ranges_read_file(struct file *file,
 	p = 0;
 	mutex_lock(&map->cache_lock);
 	list_for_each_entry(c, &map->debugfs_off_cache, list) {
-		snprintf(entry, PAGE_SIZE, "%x-%x",
-			 c->base_reg, c->max_reg);
+		entry_len = snprintf(entry, entry_size, "%x-%x\n",
+				     c->base_reg, c->max_reg);
 		if (p >= *ppos) {
-			if (buf_pos + 1 + strlen(entry) > count)
+			if (buf_pos + entry_len > count)
 				break;
-			snprintf(buf + buf_pos, count - buf_pos,
-				 "%s", entry);
-			buf_pos += strlen(entry);
-			buf[buf_pos] = '\n';
-			buf_pos++;
+			memcpy(buf + buf_pos, entry, entry_len);
+			buf_pos += entry_len;
 		}
-		p += strlen(entry) + 1;
+		p += entry_len;
 	}
 	mutex_unlock(&map->cache_lock);
 
-- 
2.1.3

Re: [PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read_file()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On Tue, Sep 29, 2015 at 12:29:02AM +0200, Rasmus Villemoes wrote:

This patch is an example of why SubmittingPatches recommends splitting
things up into one change per patch, it would be much easier to read and
review as a series (especially given that there's very few collisions).

> * A page is a bit much for two integers and a bit of punctuation. 64
>   bytes should suffice.

Right, the reason PAGE_SIZE was chosen is that it's a natural unit for
the allocator and is clearly absurdly large for the data.  For 64 bytes
I have to think for a moment if it's suitably large.  Please leave it at
PAGE_SIZE, it's not like this hangs around for any length of time.

> * Calling strlen() on entry no less than three times is silly,
>   especially when snprintf() has returned that value (which was just
>   thrown away).

I think we were expecting the compiler would figure out that strlen() is
a pure function and do the right thing here (though I do see it's
missing an annotation).

> * Transferring entry to the output buffer using snprintf is silly,
>   when we know the length. Use memcpy instead.

Right, that's a legacy of a previous version transferring the maximum
amount of data possible (which got abandoned due to complexity).
However with the changes to use the return value of snprinf() it seems
like the best thing to do here is to go back to this and just fill up as
much of the buffer as we can by using snprintf() to do the transfer.

That said I think memcpy() is going to be the best way of getting to
that since one of the issues there (which currently doesn't work) is
slicing things off the front and memcpy() handles that nicely.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read_file()

[Rasmus Villemoes]

On Tue, Sep 29 2015, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Sep 29, 2015 at 12:29:02AM +0200, Rasmus Villemoes wrote:
>
> This patch is an example of why SubmittingPatches recommends splitting
> things up into one change per patch, it would be much easier to read and
> review as a series (especially given that there's very few collisions).

It's a balance. Splitting into too small pieces and one gets accused of
trying to inflate one's commit stats. But I actually agree with you (and
SubmittingPatches), and it would be up to the maintainer to squash the
patches if they're deemed too small.

>> * A page is a bit much for two integers and a bit of punctuation. 64
>>   bytes should suffice.
>
> Right, the reason PAGE_SIZE was chosen is that it's a natural unit for
> the allocator and is clearly absurdly large for the data.  For 64 bytes
> I have to think for a moment if it's suitably large.  Please leave it at
> PAGE_SIZE, it's not like this hangs around for any length of time.

OK, but it's not that one-sided. When I read the code, I spent time
thinking if there might be any case where we'd need anywhere near that
much (which also requires checking all places that entry is used in the
function). Absurd choices can also be confusing.

>> * Calling strlen() on entry no less than three times is silly,
>>   especially when snprintf() has returned that value (which was just
>>   thrown away).
>
> I think we were expecting the compiler would figure out that strlen() is
> a pure function and do the right thing here (though I do see it's
> missing an annotation).

It does, and there's no need for an annotation - gcc knows about the
properties and semantics of the standard library string functions, which
is why it can optimize strlen("foo") to 3 at compile-time, !strlen(s) to
"*s == '\0'", using "i < strlen(s)" in the terminating condition of a for loop
doesn't always generate horrible code, etc.

HOWEVER, once there are calls to external functions in-between, all bets
are off. entry is a pointer to some blob of global memory, and the
compiler has absolutely no way of knowing whether some callee changes
that memory. So it dutifully does what the programmer told it, which is
to compute the strlen() at that point in the code. (Again, even calling
it _once_ is one more than necessary.)

Aside: This was in fact how my attention was drawn to regmap-debugfs in the
first place: I have a patch adding __attribute__((assume_aligned)) (gcc 4.9+)
annotations to kmalloc and friends. This generally lead to smaller and
more efficient code, but bloat-o-meter said that several functions in
regmap-debugfs.c grew by several hundred bytes. This turned out to be
caused by gcc deciding to open-code strlen() [using the
check-four-bytes-at-a-time-for-a-0 trick] when it knew that the string
is sufficiently aligned. Since these strlen calls are entirely
redundant, I wanted to try to get rid of those, but then I found the
stuff which was slightly more important than reducing .text and saving a
few cycles.

>> * Transferring entry to the output buffer using snprintf is silly,
>>   when we know the length. Use memcpy instead.
>
> Right, that's a legacy of a previous version transferring the maximum
> amount of data possible (which got abandoned due to complexity).
> However with the changes to use the return value of snprinf() it seems
> like the best thing to do here is to go back to this and just fill up as
> much of the buffer as we can by using snprintf() to do the transfer.
>
> That said I think memcpy() is going to be the best way of getting to
> that since one of the issues there (which currently doesn't work) is
> slicing things off the front and memcpy() handles that nicely.

I'm not sure I understand what you want to do. Should I resend as three
separate patches (the four bullets minus the PAGE_SIZE thing), and then
we can take it from there?

Rasmus

Re: [PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read_file()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On Wed, Sep 30, 2015 at 11:51:33AM +0200, Rasmus Villemoes wrote:
> On Tue, Sep 29 2015, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Sep 29, 2015 at 12:29:02AM +0200, Rasmus Villemoes wrote:

> > That said I think memcpy() is going to be the best way of getting to
> > that since one of the issues there (which currently doesn't work) is
> > slicing things off the front and memcpy() handles that nicely.

> I'm not sure I understand what you want to do. Should I resend as three
> separate patches (the four bullets minus the PAGE_SIZE thing), and then
> we can take it from there?

I'd like to while we're at optimising the code to make it more
functional by supporting copying out any chunk of data rather than just
complete lines as now.

In any case, please split up and resend as you suggest above.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2 1/3] regmap: debugfs: use snprintf return value in regmap_reg_ranges_read_file()

[Rasmus Villemoes]

Calling strlen() no less than three times on entry is silly. Since
we're formatting into a buffer with plenty of room, there's no chance
of truncation, so snprintf() has actually returned the value we want,
meaning we don't even have to call strlen once.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/base/regmap/regmap-debugfs.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index f42f2bac6466..d26b9c3ab812 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -339,6 +339,7 @@ static ssize_t regmap_reg_ranges_read_file(struct file *file,
 	char *buf;
 	char *entry;
 	int ret;
+	unsigned entry_len;
 
 	if (*ppos < 0 || !count)
 		return -EINVAL;
@@ -366,18 +367,18 @@ static ssize_t regmap_reg_ranges_read_file(struct file *file,
 	p = 0;
 	mutex_lock(&map->cache_lock);
 	list_for_each_entry(c, &map->debugfs_off_cache, list) {
-		snprintf(entry, PAGE_SIZE, "%x-%x",
-			 c->base_reg, c->max_reg);
+		entry_len = snprintf(entry, PAGE_SIZE, "%x-%x",
+				     c->base_reg, c->max_reg);
 		if (p >= *ppos) {
-			if (buf_pos + 1 + strlen(entry) > count)
+			if (buf_pos + 1 + entry_len > count)
 				break;
 			snprintf(buf + buf_pos, count - buf_pos,
 				 "%s", entry);
-			buf_pos += strlen(entry);
+			buf_pos += entry_len;
 			buf[buf_pos] = '\n';
 			buf_pos++;
 		}
-		p += strlen(entry) + 1;
+		p += entry_len + 1;
 	}
 	mutex_unlock(&map->cache_lock);
 
-- 
2.1.3

[PATCH v2 2/3] regmap: debugfs: use memcpy instead of snprintf

[Rasmus Villemoes]

Since we know the length of entry and that there's room enough in the
output buffer, using memcpy instead of snprintf is simpler and
cheaper.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/base/regmap/regmap-debugfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index d26b9c3ab812..bed690740efa 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -372,8 +372,7 @@ static ssize_t regmap_reg_ranges_read_file(struct file *file,
 		if (p >= *ppos) {
 			if (buf_pos + 1 + entry_len > count)
 				break;
-			snprintf(buf + buf_pos, count - buf_pos,
-				 "%s", entry);
+			memcpy(buf + buf_pos, entry, entry_len);
 			buf_pos += entry_len;
 			buf[buf_pos] = '\n';
 			buf_pos++;
-- 
2.1.3

[PATCH v2 3/3] regmap: debugfs: simplify regmap_reg_ranges_read_file() slightly

[Rasmus Villemoes]

By printing the newline character to entry, we can avoid accounting
for it manually in several places.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/base/regmap/regmap-debugfs.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index bed690740efa..df0d2cd60e39 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -367,17 +367,15 @@ static ssize_t regmap_reg_ranges_read_file(struct file *file,
 	p = 0;
 	mutex_lock(&map->cache_lock);
 	list_for_each_entry(c, &map->debugfs_off_cache, list) {
-		entry_len = snprintf(entry, PAGE_SIZE, "%x-%x",
+		entry_len = snprintf(entry, PAGE_SIZE, "%x-%x\n",
 				     c->base_reg, c->max_reg);
 		if (p >= *ppos) {
-			if (buf_pos + 1 + entry_len > count)
+			if (buf_pos + entry_len > count)
 				break;
 			memcpy(buf + buf_pos, entry, entry_len);
 			buf_pos += entry_len;
-			buf[buf_pos] = '\n';
-			buf_pos++;
 		}
-		p += entry_len + 1;
+		p += entry_len;
 	}
 	mutex_unlock(&map->cache_lock);
 
-- 
2.1.3

Re: [PATCH 1/2] regmap: debugfs: remove bogus check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Tue, Sep 29, 2015 at 12:29:01AM +0200, Rasmus Villemoes wrote:
> snprintf cannot return a negative value (unless map->dev->driver->name
> happens to be over 2G long). So remove the bogus check.

>  	ret = snprintf(buf, PAGE_SIZE, "%s\n", map->dev->driver->name);
> -	if (ret < 0) {
> -		kfree(buf);
> -		return ret;
> -	}

It's hard to see a great value in removing error checking, even error
checking that's currently unlikely to ever trigger...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regmap: debugfs: remove bogus check

[Rasmus Villemoes]

On Tue, Sep 29 2015, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Sep 29, 2015 at 12:29:01AM +0200, Rasmus Villemoes wrote:
>> snprintf cannot return a negative value (unless map->dev->driver->name
>> happens to be over 2G long). So remove the bogus check.
>
>>  	ret = snprintf(buf, PAGE_SIZE, "%s\n", map->dev->driver->name);
>> -	if (ret < 0) {
>> -		kfree(buf);
>> -		return ret;
>> -	}
>
> It's hard to see a great value in removing error checking, even error
> checking that's currently unlikely to ever trigger...

I agree, but only on the word 'great'. There is value in removing such
bogosities (or rather, their presence provides negative value). It makes
the code harder to read ("why is this instance checked, but not any of
the other snprintfs?"); people may think that it's trying to check for
truncation, but it does no such thing; and it contributes a few
worthless bytes to .text (and the source).

If you're worried about map->dev->driver->name actually ever being > 2G,
returning some almost totally random negative number isn't really
helpful (the function is supposed to return a -errno). And what makes
you think that in some hypothetical universe where the kernel's snprintf
explicit returns a negative value that it wouldn't just return -1 (aka
-EPERM)?

BTW, if it's the truncation thing this was supposed to check for and
there was any non-zero chance map->dev->driver->name would be longer
than around PAGE_SIZE, this would be an excellent info leak: ret is some
fine positive number somewhat larger than PAGE_SIZE, and we go on to use
that to determine how much to copy to the user.

Rasmus

Re: [PATCH 1/2] regmap: debugfs: remove bogus check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Wed, Sep 30, 2015 at 09:27:38AM +0200, Rasmus Villemoes wrote:

> I agree, but only on the word 'great'. There is value in removing such
> bogosities (or rather, their presence provides negative value). It makes
> the code harder to read ("why is this instance checked, but not any of
> the other snprintfs?"); people may think that it's trying to check for
> truncation, but it does no such thing; and it contributes a few
> worthless bytes to .text (and the source).

The solution to partial error checking isn't always to remove the error
checking!

> If you're worried about map->dev->driver->name actually ever being > 2G,
> returning some almost totally random negative number isn't really
> helpful (the function is supposed to return a -errno). And what makes
> you think that in some hypothetical universe where the kernel's snprintf
> explicit returns a negative value that it wouldn't just return -1 (aka
> -EPERM)?

This is going back to the discussion about having to learn the specific
snprinf() implementation one is dealing with.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] regmap: debugfs: remove bogus check

[Rasmus Villemoes]

On Wed, Sep 30 2015, Mark Brown <broonie@kernel.org> wrote:

> On Wed, Sep 30, 2015 at 09:27:38AM +0200, Rasmus Villemoes wrote:
>
>> I agree, but only on the word 'great'. There is value in removing such
>> bogosities (or rather, their presence provides negative value). It makes
>> the code harder to read ("why is this instance checked, but not any of
>> the other snprintfs?"); people may think that it's trying to check for
>> truncation, but it does no such thing; and it contributes a few
>> worthless bytes to .text (and the source).
>
> The solution to partial error checking isn't always to remove the error
> checking!

Since this particular piece of code is not, in fact, doing any kind of error
checking, I fail to see how that argument is relevant.

Or, more constructively: Would you please explain exactly what error
checking you think this should/could be doing? Truncation? Invalid
format string? Cosmic-ray-detected?

>> If you're worried about map->dev->driver->name actually ever being > 2G,
>> returning some almost totally random negative number isn't really
>> helpful (the function is supposed to return a -errno). And what makes
>> you think that in some hypothetical universe where the kernel's snprintf
>> explicit returns a negative value that it wouldn't just return -1 (aka
>> -EPERM)?
>
> This is going back to the discussion about having to learn the specific
> snprinf() implementation one is dealing with.

You better know what values it might return under which circumstances if
you're going to forward those as error codes to your caller.

Rasmus

Re: [PATCH 1/2] regmap: debugfs: remove bogus check

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]

On Wed, Sep 30, 2015 at 07:52:26PM +0200, Rasmus Villemoes wrote:

> Or, more constructively: Would you please explain exactly what error
> checking you think this should/could be doing? Truncation? Invalid
> format string? Cosmic-ray-detected?

Yeah, invalid format strings mainly.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]