[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 2/6] drm/nouveau/core: 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/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);
-- 
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

[PATCH 5/6] staging: speakup: replace spk_strlwr() with strtolower()

[Markus Mayer]

After introducing generic strtolower(), spk_strlwr() is no longer
needed.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/staging/speakup/kobjects.c    |  2 +-
 drivers/staging/speakup/main.c        |  2 +-
 drivers/staging/speakup/speakup.h     |  1 -
 drivers/staging/speakup/varhandlers.c | 12 ------------
 4 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
index 528cbdc..009c0d2 100644
--- a/drivers/staging/speakup/kobjects.c
+++ b/drivers/staging/speakup/kobjects.c
@@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (new_synth_name[len - 1] == '\n')
 		len--;
 	new_synth_name[len] = '\0';
-	spk_strlwr(new_synth_name);
+	strtolower(new_synth_name);
 	if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
 		pr_warn("%s already in use\n", new_synth_name);
 	} else if (synth_init(new_synth_name) != 0) {
diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
index 97ca4ec..3f3f254 100644
--- a/drivers/staging/speakup/main.c
+++ b/drivers/staging/speakup/main.c
@@ -2314,7 +2314,7 @@ static int __init speakup_init(void)
 	spk_initialize_msgs();	/* Initialize arrays for i18n. */
 	spk_reset_default_chars();
 	spk_reset_default_chartab();
-	spk_strlwr(synth_name);
+	strtolower(synth_name);
 	spk_vars[0].u.n.high = vc->vc_cols;
 	for (var = spk_vars; var->var_id != MAXVARS; var++)
 		speakup_register_var(var);
diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
index df74c91..4725785 100644
--- a/drivers/staging/speakup/speakup.h
+++ b/drivers/staging/speakup/speakup.h
@@ -50,7 +50,6 @@ void synth_insert_next_index(int sent_num);
 void spk_reset_index_count(int sc);
 void spk_get_index_count(int *linecount, int *sentcount);
 int spk_set_key_info(const u_char *key_info, u_char *k_buffer);
-char *spk_strlwr(char *s);
 char *spk_s2uchar(char *start, char *dest);
 int speakup_kobj_init(void);
 void speakup_kobj_exit(void);
diff --git a/drivers/staging/speakup/varhandlers.c b/drivers/staging/speakup/varhandlers.c
index e1393d2..a1af222 100644
--- a/drivers/staging/speakup/varhandlers.c
+++ b/drivers/staging/speakup/varhandlers.c
@@ -309,18 +309,6 @@ int spk_set_mask_bits(const char *input, const int which, const int how)
 	return 0;
 }
 
-char *spk_strlwr(char *s)
-{
-	char *p;
-
-	if (!s)
-		return NULL;
-
-	for (p = s; *p; p++)
-		*p = tolower(*p);
-	return s;
-}
-
 char *spk_s2uchar(char *start, char *dest)
 {
 	int val;
-- 
2.7.4

[PATCH 6/6] iscsi-target: replace iscsi_initiatorname_tolower() with strtolower()

[Markus Mayer]

After introducing generic strtolower(), iscsi_initiatorname_tolower()
is no longer needed.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/target/iscsi/iscsi_target_nego.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 89d34bd..fa20638 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -987,21 +987,6 @@ static int iscsi_target_do_login(struct iscsi_conn *conn, struct iscsi_login *lo
 	return 0;
 }
 
-static void iscsi_initiatorname_tolower(
-	char *param_buf)
-{
-	char *c;
-	u32 iqn_size = strlen(param_buf), i;
-
-	for (i = 0; i < iqn_size; i++) {
-		c = &param_buf[i];
-		if (!isupper(*c))
-			continue;
-
-		*c = tolower(*c);
-	}
-}
-
 /*
  * Processes the first Login Request..
  */
@@ -1075,7 +1060,7 @@ int iscsi_target_locate_portal(
 	 * RFC-3720 3.2.6.1. section c) that says that iSCSI IQNs
 	 * are NOT case sensitive.
 	 */
-	iscsi_initiatorname_tolower(i_buf);
+	strtolower(i_buf);
 
 	if (!s_buf) {
 		if (!login->leading_connection)
-- 
2.7.4

Re: [PATCH 5/6] staging: speakup: replace spk_strlwr() with strtolower()

[Samuel Thibault]

Markus Mayer, on Thu 30 Jun 2016 16:50:11 -0700, wrote:
> After introducing generic strtolower(), spk_strlwr() is no longer
> needed.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  drivers/staging/speakup/kobjects.c    |  2 +-
>  drivers/staging/speakup/main.c        |  2 +-
>  drivers/staging/speakup/speakup.h     |  1 -
>  drivers/staging/speakup/varhandlers.c | 12 ------------
>  4 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/speakup/kobjects.c b/drivers/staging/speakup/kobjects.c
> index 528cbdc..009c0d2 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	if (new_synth_name[len - 1] == '\n')
>  		len--;
>  	new_synth_name[len] = '\0';
> -	spk_strlwr(new_synth_name);
> +	strtolower(new_synth_name);
>  	if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
>  		pr_warn("%s already in use\n", new_synth_name);
>  	} else if (synth_init(new_synth_name) != 0) {
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index 97ca4ec..3f3f254 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -2314,7 +2314,7 @@ static int __init speakup_init(void)
>  	spk_initialize_msgs();	/* Initialize arrays for i18n. */
>  	spk_reset_default_chars();
>  	spk_reset_default_chartab();
> -	spk_strlwr(synth_name);
> +	strtolower(synth_name);
>  	spk_vars[0].u.n.high = vc->vc_cols;
>  	for (var = spk_vars; var->var_id != MAXVARS; var++)
>  		speakup_register_var(var);
> diff --git a/drivers/staging/speakup/speakup.h b/drivers/staging/speakup/speakup.h
> index df74c91..4725785 100644
> --- a/drivers/staging/speakup/speakup.h
> +++ b/drivers/staging/speakup/speakup.h
> @@ -50,7 +50,6 @@ void synth_insert_next_index(int sent_num);
>  void spk_reset_index_count(int sc);
>  void spk_get_index_count(int *linecount, int *sentcount);
>  int spk_set_key_info(const u_char *key_info, u_char *k_buffer);
> -char *spk_strlwr(char *s);
>  char *spk_s2uchar(char *start, char *dest);
>  int speakup_kobj_init(void);
>  void speakup_kobj_exit(void);
> diff --git a/drivers/staging/speakup/varhandlers.c b/drivers/staging/speakup/varhandlers.c
> index e1393d2..a1af222 100644
> --- a/drivers/staging/speakup/varhandlers.c
> +++ b/drivers/staging/speakup/varhandlers.c
> @@ -309,18 +309,6 @@ int spk_set_mask_bits(const char *input, const int which, const int how)
>  	return 0;
>  }
>  
> -char *spk_strlwr(char *s)
> -{
> -	char *p;
> -
> -	if (!s)
> -		return NULL;
> -
> -	for (p = s; *p; p++)
> -		*p = tolower(*p);
> -	return s;
> -}
> -
>  char *spk_s2uchar(char *start, char *dest)
>  {
>  	int val;
> -- 
> 2.7.4
> 

-- 
Samuel
<D> m'enfin, le 5 juillet, le mec vient visiter le labo...
* D a marque d'une croix rouge le 5 juillet sur son agenda
<y> niarc niarc niarc
<D> cet homme va souffrir
<B> c'est donc le 5 juillet qu'il meurt d'un accident de la route écrasé par un truck muni d'un pare buffle
 -+- #ens-mim - repaire de terroristes -+-

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

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

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: [Nouveau] [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function

[Alexandre Courbot]

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?

Re: [Nouveau] [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function

[Markus Mayer]

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

Re: [Nouveau] [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function

[Alexandre Courbot]

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.

Re: [Nouveau] [PATCH 2/6] drm/nouveau/core: make use of new strtolower() function

[Alexandre Courbot]

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.

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