[PATCH 0/6] lib: string: add function strtolower()

[PATCH 0/6] lib: string: add function strtolower()

[Markus Mayer]

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(-)

-- 
2.7.4

[PATCH 1/6] lib: string: add function strtolower()

[Markus Mayer]

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);
-- 
2.7.4

[PATCH 3/6] ACPICA: make use of new strtolower() function

[Markus Mayer]

Call strtolower() rather than walking the string explicitly to convert
it to lowercase.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---

*** Please note that there don't seem to be any callers of acpi_ut_strlwr().
*** It may be possible to remove the function altogether.

 drivers/acpi/acpica/utnonansi.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/acpi/acpica/utnonansi.c b/drivers/acpi/acpica/utnonansi.c
index 3465fe2..b6e11dc 100644
--- a/drivers/acpi/acpica/utnonansi.c
+++ b/drivers/acpi/acpica/utnonansi.c
@@ -64,19 +64,8 @@ ACPI_MODULE_NAME("utnonansi")
  ******************************************************************************/
 void acpi_ut_strlwr(char *src_string)
 {
-	char *string;
-
 	ACPI_FUNCTION_ENTRY();
-
-	if (!src_string) {
-		return;
-	}
-
-	/* Walk entire string, lowercasing the letters */
-
-	for (string = src_string; *string; string++) {
-		*string = (char)tolower((int)*string);
-	}
+	strtolower(src_string);
 }
 
 /*******************************************************************************
-- 
2.7.4

[PATCH 4/6] ACPI / device_sysfs: make use of new strtolower() function

[Markus Mayer]

Call strtolower() rather than walking the string explicitly to convert
it to lowercase.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/acpi/device_sysfs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 7b2c48f..1db38c7 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -200,12 +200,10 @@ static int create_of_modalias(struct acpi_device *acpi_dev, char *modalias,
 	const union acpi_object *of_compatible, *obj;
 	int len, count;
 	int i, nval;
-	char *c;
 
 	acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
 	/* DT strings are all in lower case */
-	for (c = buf.pointer; *c != '\0'; c++)
-		*c = tolower(*c);
+	strtolower(buf.pointer);
 
 	len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
 	ACPI_FREE(buf.pointer);
-- 
2.7.4

RE: [PATCH 3/6] ACPICA: make use of new strtolower() function

[Moore, Robert]

Where is "strtolower" implemented?


> -----Original Message-----
> From: Markus Mayer [mailto:mmayer@broadcom.com]
> Sent: Thursday, June 30, 2016 4:50 PM
> To: Moore, Robert; Zheng, Lv; Wysocki, Rafael J; Len Brown
> Cc: Markus Mayer; linux-acpi@vger.kernel.org; devel@acpica.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH 3/6] ACPICA: make use of new strtolower() function
> 
> Call strtolower() rather than walking the string explicitly to convert it
> to lowercase.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
> 
> *** Please note that there don't seem to be any callers of
> acpi_ut_strlwr().
> *** It may be possible to remove the function altogether.
> 
>  drivers/acpi/acpica/utnonansi.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/utnonansi.c
> b/drivers/acpi/acpica/utnonansi.c index 3465fe2..b6e11dc 100644
> --- a/drivers/acpi/acpica/utnonansi.c
> +++ b/drivers/acpi/acpica/utnonansi.c
> @@ -64,19 +64,8 @@ ACPI_MODULE_NAME("utnonansi")
> 
> **************************************************************************
> ****/
>  void acpi_ut_strlwr(char *src_string)
>  {
> -	char *string;
> -
>  	ACPI_FUNCTION_ENTRY();
> -
> -	if (!src_string) {
> -		return;
> -	}
> -
> -	/* Walk entire string, lowercasing the letters */
> -
> -	for (string = src_string; *string; string++) {
> -		*string = (char)tolower((int)*string);
> -	}
> +	strtolower(src_string);
>  }
> 
> 
> /*************************************************************************
> ******
> --
> 2.7.4

Re: [PATCH 3/6] ACPICA: make use of new strtolower() function

[Markus Mayer]

On 30 June 2016 at 19:59, Moore, Robert <robert.moore@intel.com> wrote:
> This is linux-specific code, ACPICA is os-independent. So we cannot accept
> such patch.

Understood. I wasn't aware that this was shared code.

> From: Markus Mayer [mailto:markus.mayer@broadcom.com]
> Sent: Thursday, June 30, 2016 7:50 PM
> To: Moore, Robert
> Cc: Zheng, Lv; Wysocki, Rafael J; Len Brown; linux-acpi@vger.kernel.org;
> devel@acpica.org; linux-kernel@vger.kernel.org; Box, David E
> Subject: Re: [PATCH 3/6] ACPICA: make use of new strtolower() function
>
> On Thursday, 30 June 2016, Moore, Robert <robert.moore@intel.com> wrote:
>
> Where is "strtolower" implemented?
>
> First patch of the series:
>
> https://lkml.org/lkml/2016/6/30/733
>
>> -----Original Message-----
>> From: Markus Mayer [mailto:mmayer@broadcom.com]
>> Sent: Thursday, June 30, 2016 4:50 PM
>> To: Moore, Robert; Zheng, Lv; Wysocki, Rafael J; Len Brown
>> Cc: Markus Mayer; linux-acpi@vger.kernel.org; devel@acpica.org; linux-
>> kernel@vger.kernel.org
>> Subject: [PATCH 3/6] ACPICA: make use of new strtolower() function
>>
>> Call strtolower() rather than walking the string explicitly to convert it
>> to lowercase.
>>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>
>> *** Please note that there don't seem to be any callers of
>> acpi_ut_strlwr().
>> *** It may be possible to remove the function altogether.
>>
>>  drivers/acpi/acpica/utnonansi.c | 13 +------------
>>  1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/drivers/acpi/acpica/utnonansi.c
>> b/drivers/acpi/acpica/utnonansi.c index 3465fe2..b6e11dc 100644
>> --- a/drivers/acpi/acpica/utnonansi.c
>> +++ b/drivers/acpi/acpica/utnonansi.c
>> @@ -64,19 +64,8 @@ ACPI_MODULE_NAME("utnonansi")
>>
>> **************************************************************************
>> ****/
>>  void acpi_ut_strlwr(char *src_string)
>>  {
>> -     char *string;
>> -
>>       ACPI_FUNCTION_ENTRY();
>> -
>> -     if (!src_string) {
>> -             return;
>> -     }
>> -
>> -     /* Walk entire string, lowercasing the letters */
>> -
>> -     for (string = src_string; *string; string++) {
>> -             *string = (char)tolower((int)*string);
>> -     }
>> +     strtolower(src_string);
>>  }
>>
>>
>> /*************************************************************************
>> ******
>> --
>> 2.7.4

RE: [PATCH 3/6] ACPICA: make use of new strtolower() function

[Moore, Robert]

> -----Original Message-----
> From: Markus Mayer [mailto:markus.mayer@broadcom.com]
> Sent: Thursday, June 30, 2016 9:13 PM
> To: Moore, Robert
> Cc: Zheng, Lv; Wysocki, Rafael J; Len Brown; linux-acpi@vger.kernel.org;
> devel@acpica.org; linux-kernel@vger.kernel.org; Box, David E
> Subject: Re: [PATCH 3/6] ACPICA: make use of new strtolower() function
> 
> On 30 June 2016 at 19:59, Moore, Robert <robert.moore@intel.com> wrote:
> > This is linux-specific code, ACPICA is os-independent. So we cannot
> > accept such patch.
> 
> Understood. I wasn't aware that this was shared code.

Ok. Glad to take fixes and optimizations, however.
Bob



> 
> > From: Markus Mayer [mailto:markus.mayer@broadcom.com]
> > Sent: Thursday, June 30, 2016 7:50 PM
> > To: Moore, Robert
> > Cc: Zheng, Lv; Wysocki, Rafael J; Len Brown;
> > linux-acpi@vger.kernel.org; devel@acpica.org;
> > linux-kernel@vger.kernel.org; Box, David E
> > Subject: Re: [PATCH 3/6] ACPICA: make use of new strtolower() function
> >
> > On Thursday, 30 June 2016, Moore, Robert <robert.moore@intel.com> wrote:
> >
> > Where is "strtolower" implemented?
> >
> > First patch of the series:
> >
> > https://lkml.org/lkml/2016/6/30/733
> >
> >> -----Original Message-----
> >> From: Markus Mayer [mailto:mmayer@broadcom.com]
> >> Sent: Thursday, June 30, 2016 4:50 PM
> >> To: Moore, Robert; Zheng, Lv; Wysocki, Rafael J; Len Brown
> >> Cc: Markus Mayer; linux-acpi@vger.kernel.org; devel@acpica.org;
> >> linux- kernel@vger.kernel.org
> >> Subject: [PATCH 3/6] ACPICA: make use of new strtolower() function
> >>
> >> Call strtolower() rather than walking the string explicitly to
> >> convert it to lowercase.
> >>
> >> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> >> ---
> >>
> >> *** Please note that there don't seem to be any callers of
> >> acpi_ut_strlwr().
> >> *** It may be possible to remove the function altogether.
> >>
> >>  drivers/acpi/acpica/utnonansi.c | 13 +------------
> >>  1 file changed, 1 insertion(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/acpi/acpica/utnonansi.c
> >> b/drivers/acpi/acpica/utnonansi.c index 3465fe2..b6e11dc 100644
> >> --- a/drivers/acpi/acpica/utnonansi.c
> >> +++ b/drivers/acpi/acpica/utnonansi.c
> >> @@ -64,19 +64,8 @@ ACPI_MODULE_NAME("utnonansi")
> >>
> >> *********************************************************************
> >> *****
> >> ****/
> >>  void acpi_ut_strlwr(char *src_string)  {
> >> -     char *string;
> >> -
> >>       ACPI_FUNCTION_ENTRY();
> >> -
> >> -     if (!src_string) {
> >> -             return;
> >> -     }
> >> -
> >> -     /* Walk entire string, lowercasing the letters */
> >> -
> >> -     for (string = src_string; *string; string++) {
> >> -             *string = (char)tolower((int)*string);
> >> -     }
> >> +     strtolower(src_string);
> >>  }
> >>
> >>
> >> /********************************************************************
> >> *****
> >> ******
> >> --
> >> 2.7.4

Re: [PATCH 1/6] lib: string: add function strtolower()

[Jani Nikula]

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);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] lib: string: add function strtolower()

[Markus Mayer]

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

Re: [PATCH 1/6] lib: string: add function strtolower()

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] ACPI / device_sysfs: make use of new strtolower() function

[Rafael J. Wysocki]

On Thursday, June 30, 2016 04:50:10 PM Markus Mayer wrote:
> Call strtolower() rather than walking the string explicitly to convert
> it to lowercase.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>

ACK

> ---
>  drivers/acpi/device_sysfs.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index 7b2c48f..1db38c7 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -200,12 +200,10 @@ static int create_of_modalias(struct acpi_device *acpi_dev, char *modalias,
>  	const union acpi_object *of_compatible, *obj;
>  	int len, count;
>  	int i, nval;
> -	char *c;
>  
>  	acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
>  	/* DT strings are all in lower case */
> -	for (c = buf.pointer; *c != '\0'; c++)
> -		*c = tolower(*c);
> +	strtolower(buf.pointer);
>  
>  	len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
>  	ACPI_FREE(buf.pointer);
> 

Re: [PATCH 1/6] lib: string: add function strtolower()

[Rasmus Villemoes]

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

Re: [PATCH 1/6] lib: string: add function strtolower()

[Markus Mayer]

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