This series introduces a new generic function strtolower(), which converts strings to lowercase in-place, overwriting the original string. This kind of functionality is needed in several places in the kernel. Right now, everybody seems to be implementing their own copy of this function. So, we replace several custom "strtolower" implementations with this new library function.
Another driver that also makes use of this function will be submitted upstream shortly, which prompted this whole exercise.
The changes made here have been compile-tested, but not tried out, due to lack of required hardware.
This series is based on v4.7-rc5.
Markus Mayer (6): lib: string: add function strtolower() drm/nouveau/core: make use of new strtolower() function ACPICA: make use of new strtolower() function ACPI / device_sysfs: make use of new strtolower() function staging: speakup: replace spk_strlwr() with strtolower() iscsi-target: replace iscsi_initiatorname_tolower() with strtolower()
drivers/acpi/acpica/utnonansi.c | 13 +------------ drivers/acpi/device_sysfs.c | 4 +--- drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------ drivers/staging/speakup/kobjects.c | 2 +- drivers/staging/speakup/main.c | 2 +- drivers/staging/speakup/speakup.h | 1 - drivers/staging/speakup/varhandlers.c | 12 ------------ drivers/target/iscsi/iscsi_target_nego.c | 17 +---------------- include/linux/string.h | 1 + lib/string.c | 14 ++++++++++++++ 10 files changed, 21 insertions(+), 52 deletions(-)
Add a function called strtolower() to convert strings to lower case in-place, overwriting the original string.
This seems to be a recurring requirement in the kernel that is currently being solved by several duplicated implementations doing the same thing.
Signed-off-by: Markus Mayer mmayer@broadcom.com --- include/linux/string.h | 1 + lib/string.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 26b6f6a..aad605e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); #endif void *memchr_inv(const void *s, int c, size_t n); char *strreplace(char *s, char old, char new); +char *strtolower(char *s);
extern void kfree_const(const void *x);
diff --git a/lib/string.c b/lib/string.c index ed83562..6e3b560 100644 --- a/lib/string.c +++ b/lib/string.c @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) return s; } EXPORT_SYMBOL(strreplace); + +char *strtolower(char *s) +{ + char *p; + + if (unlikely(!s)) + return NULL; + + for (p = s; *p; p++) + *p = tolower(*p); + + return s; +} +EXPORT_SYMBOL(strtolower);
On Fri, 01 Jul 2016, Markus Mayer mmayer@broadcom.com wrote:
Add a function called strtolower() to convert strings to lower case in-place, overwriting the original string.
This seems to be a recurring requirement in the kernel that is currently being solved by several duplicated implementations doing the same thing.
Signed-off-by: Markus Mayer mmayer@broadcom.com
include/linux/string.h | 1 + lib/string.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 26b6f6a..aad605e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); #endif void *memchr_inv(const void *s, int c, size_t n); char *strreplace(char *s, char old, char new); +char *strtolower(char *s);
extern void kfree_const(const void *x);
diff --git a/lib/string.c b/lib/string.c index ed83562..6e3b560 100644 --- a/lib/string.c +++ b/lib/string.c @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) return s; } EXPORT_SYMBOL(strreplace);
This needs a kernel-doc comment right here.
+char *strtolower(char *s) +{
- char *p;
if (unlikely(!s))return NULL;
Using spaces for indentation? See scripts/checkpatch.pl.
- for (p = s; *p; p++)
*p = tolower(*p);- return s;
Why does it return a value? Could be void?
BR, Jani.
+} +EXPORT_SYMBOL(strtolower);
On 1 July 2016 at 03:52, Jani Nikula jani.nikula@linux.intel.com wrote:
On Fri, 01 Jul 2016, Markus Mayer mmayer@broadcom.com wrote:
Add a function called strtolower() to convert strings to lower case in-place, overwriting the original string.
This seems to be a recurring requirement in the kernel that is currently being solved by several duplicated implementations doing the same thing.
Signed-off-by: Markus Mayer mmayer@broadcom.com
include/linux/string.h | 1 + lib/string.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 26b6f6a..aad605e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); #endif void *memchr_inv(const void *s, int c, size_t n); char *strreplace(char *s, char old, char new); +char *strtolower(char *s);
extern void kfree_const(const void *x);
diff --git a/lib/string.c b/lib/string.c index ed83562..6e3b560 100644 --- a/lib/string.c +++ b/lib/string.c @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) return s; } EXPORT_SYMBOL(strreplace);
This needs a kernel-doc comment right here.
Will add it.
+char *strtolower(char *s) +{
char *p;if (unlikely(!s))return NULL;Using spaces for indentation? See scripts/checkpatch.pl.
Not on purpose. Thanks for spotting it.
for (p = s; *p; p++)*p = tolower(*p);return s;Why does it return a value? Could be void?
It could be void, but I thought that would make the function's use less flexible. As is, the return value is there if anybody wants it, but it can be ignored if it is not needed. Also, it seems customary for string functions to be returning the string that was passed in.
I'll change it to void if there are strong opinions leaning that way. Personally, I like that it returns a char * better.
BR, Jani.
+} +EXPORT_SYMBOL(strtolower);
-- Jani Nikula, Intel Open Source Technology Center
On Fri, 01 Jul 2016, Markus Mayer markus.mayer@broadcom.com wrote:
On 1 July 2016 at 03:52, Jani Nikula jani.nikula@linux.intel.com wrote:
On Fri, 01 Jul 2016, Markus Mayer mmayer@broadcom.com wrote:
Add a function called strtolower() to convert strings to lower case in-place, overwriting the original string.
This seems to be a recurring requirement in the kernel that is currently being solved by several duplicated implementations doing the same thing.
Signed-off-by: Markus Mayer mmayer@broadcom.com
include/linux/string.h | 1 + lib/string.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 26b6f6a..aad605e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); #endif void *memchr_inv(const void *s, int c, size_t n); char *strreplace(char *s, char old, char new); +char *strtolower(char *s);
extern void kfree_const(const void *x);
diff --git a/lib/string.c b/lib/string.c index ed83562..6e3b560 100644 --- a/lib/string.c +++ b/lib/string.c @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) return s; } EXPORT_SYMBOL(strreplace);
This needs a kernel-doc comment right here.
Will add it.
+char *strtolower(char *s) +{
char *p;if (unlikely(!s))return NULL;Using spaces for indentation? See scripts/checkpatch.pl.
Not on purpose. Thanks for spotting it.
for (p = s; *p; p++)*p = tolower(*p);return s;Why does it return a value? Could be void?
It could be void, but I thought that would make the function's use less flexible. As is, the return value is there if anybody wants it, but it can be ignored if it is not needed. Also, it seems customary for string functions to be returning the string that was passed in.
I'll change it to void if there are strong opinions leaning that way. Personally, I like that it returns a char * better.
I don't have strong opinions on this. Just a general aversion to returning something redundant. Avoids questions like, does it allocate a new string, should I use the return value instead of the string I passed in, should I check the return value or can I ignore it, should I check both the string I pass in and the return value for != NULL, etc. But I could be persuaded either way.
BR, Jani.
BR, Jani.
+} +EXPORT_SYMBOL(strtolower);
-- Jani Nikula, Intel Open Source Technology Center
On Fri, Jul 01 2016, Markus Mayer mmayer@broadcom.com wrote:
Add a function called strtolower() to convert strings to lower case in-place, overwriting the original string.
This seems to be a recurring requirement in the kernel that is currently being solved by several duplicated implementations doing the same thing.
Signed-off-by: Markus Mayer mmayer@broadcom.com
include/linux/string.h | 1 + lib/string.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h index 26b6f6a..aad605e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t); #endif void *memchr_inv(const void *s, int c, size_t n); char *strreplace(char *s, char old, char new); +char *strtolower(char *s);
extern void kfree_const(const void *x);
diff --git a/lib/string.c b/lib/string.c index ed83562..6e3b560 100644 --- a/lib/string.c +++ b/lib/string.c @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new) return s; } EXPORT_SYMBOL(strreplace);
+char *strtolower(char *s) +{
- char *p;
if (unlikely(!s))return NULL;- for (p = s; *p; p++)
*p = tolower(*p);- return s;
+} +EXPORT_SYMBOL(strtolower);
A few suggestions:
- Make the function take separate src and dst parameters, making it explicitly allowed to pass the same value (but not other kinds of overlap, of course). That way one can avoid "strcpy(dst, src); strtolower(dst);".
- Drop the NULL check. If someone does "foo->bar = something; strtolower(foo->bar); put foo in a global data structure...", the dereference of foo->bar may happen much later. Doing the NULL deref sooner means it's much easier to find and fix the bug. (Also, other str* and mem* functions don't usually check for NULL).
- While it's true that strcpy and memcpy by definition return dst, that's mostly useless. If you want it to return anything, please make it something that might be used - for example, having stpcpy semantics (returning a pointer to dst's terminating \0) means a caller might avoid a strlen call.
- Maybe do strtoupper while you're at it. Quick grepping didn't find any use for the copy-while-lowercasing, but copy-while-uppercasing can at least be used in drivers/acpi/acpica/nsrepair2.c, drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c, drivers/power/power_supply_sysfs.c along with a bunch of inplace uppercasing.
Rasmus
On 1 July 2016 at 14:08, Rasmus Villemoes linux@rasmusvillemoes.dk wrote:
A few suggestions:
Make the function take separate src and dst parameters, making it explicitly allowed to pass the same value (but not other kinds of overlap, of course). That way one can avoid "strcpy(dst, src); strtolower(dst);".
Drop the NULL check. If someone does "foo->bar = something; strtolower(foo->bar); put foo in a global data structure...", the dereference of foo->bar may happen much later. Doing the NULL deref sooner means it's much easier to find and fix the bug. (Also, other str* and mem* functions don't usually check for NULL).
While it's true that strcpy and memcpy by definition return dst, that's mostly useless. If you want it to return anything, please make it something that might be used - for example, having stpcpy semantics (returning a pointer to dst's terminating \0) means a caller might avoid a strlen call.
Maybe do strtoupper while you're at it. Quick grepping didn't find any use for the copy-while-lowercasing, but copy-while-uppercasing can at least be used in drivers/acpi/acpica/nsrepair2.c, drivers/gpu/drm/nouveau/nvkm/engine/fifo/gk104.c, drivers/power/power_supply_sysfs.c along with a bunch of inplace uppercasing.
Rasmus
Thanks for the suggestions to you and Jani. Based on the feedback I received, I am reworking the series now and will post v2 probably tomorrow.
Regards, -Markus
Call strtolower() rather than walking the string explicitly to convert it to lowercase.
Signed-off-by: Markus Mayer mmayer@broadcom.com --- drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c index 34ecd4a..c50594c 100644 --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname, { char f[64]; char cname[16]; - int i;
/* Convert device name to lowercase */ strncpy(cname, device->chip->name, sizeof(cname)); cname[sizeof(cname) - 1] = '\0'; - i = strlen(cname); - while (i) { - --i; - cname[i] = tolower(cname[i]); - } + strtolower(cname);
snprintf(f, sizeof(f), "nvidia/%s/%s.bin", cname, fwname); return request_firmware(fw, f, device->dev);
On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer mmayer@broadcom.com wrote:
Call strtolower() rather than walking the string explicitly to convert it to lowercase.
Signed-off-by: Markus Mayer mmayer@broadcom.com
drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c index 34ecd4a..c50594c 100644 --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname, { char f[64]; char cname[16];
int i; /* Convert device name to lowercase */ strncpy(cname, device->chip->name, sizeof(cname)); cname[sizeof(cname) - 1] = '\0';i = strlen(cname);while (i) {--i;cname[i] = tolower(cname[i]);}
strtolower(cname);
This function doesn't seem to exist as of next-20160701, where have you found it?
On 1 July 2016 at 18:18, Alexandre Courbot gnurou@gmail.com wrote:
On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer mmayer@broadcom.com wrote:
Call strtolower() rather than walking the string explicitly to convert it to lowercase.
Signed-off-by: Markus Mayer mmayer@broadcom.com
drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c index 34ecd4a..c50594c 100644 --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname, { char f[64]; char cname[16];
int i; /* Convert device name to lowercase */ strncpy(cname, device->chip->name, sizeof(cname)); cname[sizeof(cname) - 1] = '\0';i = strlen(cname);while (i) {--i;cname[i] = tolower(cname[i]);}
strtolower(cname);This function doesn't seem to exist as of next-20160701, where have you found it?
I didn't find it. I wrote it, because it didn't exist and I needed it. See: https://lkml.org/lkml/2016/6/30/727 and https://lkml.org/lkml/2016/6/30/733 (cover letter and first patch in series).
On Sun, Jul 3, 2016 at 12:21 AM, Markus Mayer markus.mayer@broadcom.com wrote:
On 1 July 2016 at 18:18, Alexandre Courbot gnurou@gmail.com wrote:
On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer mmayer@broadcom.com wrote:
Call strtolower() rather than walking the string explicitly to convert it to lowercase.
Signed-off-by: Markus Mayer mmayer@broadcom.com
drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c index 34ecd4a..c50594c 100644 --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname, { char f[64]; char cname[16];
int i; /* Convert device name to lowercase */ strncpy(cname, device->chip->name, sizeof(cname)); cname[sizeof(cname) - 1] = '\0';i = strlen(cname);while (i) {--i;cname[i] = tolower(cname[i]);}
strtolower(cname);This function doesn't seem to exist as of next-20160701, where have you found it?
I didn't find it. I wrote it, because it didn't exist and I needed it. See: https://lkml.org/lkml/2016/6/30/727 and https://lkml.org/lkml/2016/6/30/733 (cover letter and first patch in series).
Ah, right - would have been easier to understand if you had sent the whole series (or at least patches 0 to 2) to us as well. Please do that for the next version.
On Mon, Jul 4, 2016 at 10:37 AM, Alexandre Courbot gnurou@gmail.com wrote:
On Sun, Jul 3, 2016 at 12:21 AM, Markus Mayer markus.mayer@broadcom.com wrote:
On 1 July 2016 at 18:18, Alexandre Courbot gnurou@gmail.com wrote:
On Fri, Jul 1, 2016 at 8:50 AM, Markus Mayer mmayer@broadcom.com wrote:
Call strtolower() rather than walking the string explicitly to convert it to lowercase.
Signed-off-by: Markus Mayer mmayer@broadcom.com
drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c index 34ecd4a..c50594c 100644 --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c @@ -36,16 +36,11 @@ nvkm_firmware_get(struct nvkm_device *device, const char *fwname, { char f[64]; char cname[16];
int i; /* Convert device name to lowercase */ strncpy(cname, device->chip->name, sizeof(cname)); cname[sizeof(cname) - 1] = '\0';i = strlen(cname);while (i) {--i;cname[i] = tolower(cname[i]);}
strtolower(cname);This function doesn't seem to exist as of next-20160701, where have you found it?
I didn't find it. I wrote it, because it didn't exist and I needed it. See: https://lkml.org/lkml/2016/6/30/727 and https://lkml.org/lkml/2016/6/30/733 (cover letter and first patch in series).
Ah, right - would have been easier to understand if you had sent the whole series (or at least patches 0 to 2) to us as well. Please do that for the next version.
... found patches 0 and 1 in my spam folder, for some weird reason. Apologies for jumping to conclusions.
dri-devel@lists.freedesktop.org
-
Alexandre Courbot -
Jani Nikula -
Markus Mayer -
Markus Mayer -
Rasmus Villemoes